Re: Skipping logical replication transactions on subscriber side

On Wed, Nov 10, 2021 at 12:49 PM vignesh C <vignesh21@gmail.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/