Re: Skipping logical replication transactions on subscriber side

On Wed, Nov 10, 2021 at 12:49 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
>
> Thanks for the updated patch, Few comments:

Thank you for the comments!

> 1) should we change "Tables and functions hashes are initialized to
> empty" to "Tables, functions and subworker hashes are initialized to
> empty"
> + hash_ctl.keysize = sizeof(PgStat_StatSubWorkerKey);
> + hash_ctl.entrysize = sizeof(PgStat_StatSubWorkerEntry);
> + dbentry->subworkers = hash_create("Per-database subscription worker",
> +
> PGSTAT_SUBWORKER_HASH_SIZE,
> +
> &hash_ctl,
> +
> HASH_ELEM | HASH_BLOBS);

Fixed.

>
> 2) Since databaseid, tabhash, funchash and subworkerhash are members
> of dbentry, can we remove the function arguments databaseid, tabhash,
> funchash and subworkerhash and pass dbentry similar to
> pgstat_write_db_statsfile function?
> @@ -4370,12 +4582,14 @@ done:
> */
> static void
> pgstat_read_db_statsfile(Oid databaseid, HTAB *tabhash, HTAB *funchash,
> - bool permanent)
> + HTAB *subworkerhash,
> bool permanent)
> {
> PgStat_StatTabEntry *tabentry;
> PgStat_StatTabEntry tabbuf;
> PgStat_StatFuncEntry funcbuf;
> PgStat_StatFuncEntry *funcentry;
> + PgStat_StatSubWorkerEntry subwbuf;
> + PgStat_StatSubWorkerEntry *subwentry;
>

As the comment of this function says, this function has the ability to
skip storing per-table or per-function (and or
per-subscription-workers) data, if NULL is passed for the
corresponding hashtable, although that's not used at the moment. IMO
it'd be better to keep such behavior.

> 3) Can we move pgstat_get_subworker_entry below pgstat_get_db_entry
> and pgstat_get_tab_entry, so that the hash lookup can be together
> consistently. Similarly pgstat_send_subscription_purge can be moved
> after pgstat_send_slru.
> +/* ----------
> + * pgstat_get_subworker_entry
> + *
> + * Return subscription worker entry with the given subscription OID and
> + * relation OID. If subrelid is InvalidOid, it returns an entry of the
> + * apply worker otherwise of the table sync worker associated with subrelid.
> + * If no subscription entry exists, initialize it, if the create parameter
> + * is true. Else, return NULL.
> + * ----------
> + */
> +static PgStat_StatSubWorkerEntry *
> +pgstat_get_subworker_entry(PgStat_StatDBEntry *dbentry, Oid subid,
> Oid subrelid,
> + bool create)
> +{
> + PgStat_StatSubWorkerEntry *subwentry;
> + PgStat_StatSubWorkerKey key;
> + bool found;

Agreed. Moved.

>
> 4) This change can be removed from pgstat.c:
> @@ -332,9 +339,11 @@ static bool pgstat_db_requested(Oid databaseid);
> static PgStat_StatReplSlotEntry *pgstat_get_replslot_entry(NameData
> name, bool create_it);
> static void pgstat_reset_replslot(PgStat_StatReplSlotEntry
> *slotstats, TimestampTz ts);
>
> +
> static void pgstat_send_tabstat(PgStat_MsgTabstat *tsmsg, TimestampTz now);
> static void pgstat_send_funcstats(void);

Removed.

>
> 5) I was able to compile without including
> catalog/pg_subscription_rel.h, we can remove including
> catalog/pg_subscription_rel.h if not required.
> --- a/src/backend/postmaster/pgstat.c
> +++ b/src/backend/postmaster/pgstat.c
> @@ -41,6 +41,8 @@
> #include "catalog/catalog.h"
> #include "catalog/pg_database.h"
> #include "catalog/pg_proc.h"
> +#include "catalog/pg_subscription.h"
> +#include "catalog/pg_subscription_rel.h"

Removed.

>
> 6) Similarly replication/logicalproto.h also need not be included
> --- a/src/backend/utils/adt/pgstatfuncs.c
> +++ b/src/backend/utils/adt/pgstatfuncs.c
> @@ -24,6 +24,7 @@
> #include "pgstat.h"
> #include "postmaster/bgworker_internals.h"
> #include "postmaster/postmaster.h"
> +#include "replication/logicalproto.h"
> #include "replication/slot.h"
> #include "storage/proc.h"

Removed;

>
> 7) There is an extra ";", We can remove one ";" from below:
> + PgStat_StatSubWorkerKey key;
> + bool found;
> + HASHACTION action = (create ? HASH_ENTER : HASH_FIND);;
> +
> + key.subid = subid;
> + key.subrelid = subrelid;

Fixed.

I've attached an updated patch that incorporates all comments I got so
far. Please review it.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/