RE: row filtering for logical replication

On Mon, Jan 17, 2022 12:34 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Here are some review comments for v65-0001 (review of updates since
> v64-0001)

Thanks for the comments!

> ~~~
>
> 1. src/include/commands/publicationcmds.h - rename func
>
> +extern bool contain_invalid_rfcolumn(Oid pubid, Relation relation,
> +List *ancestors, AttrNumber *invalid_rfcolumn);
>
> I thought that function should be called "contains_..." instead of "contain_...".
>
> ~~~
>
> 2. src/backend/commands/publicationcmds.c - rename funcs
>
> Suggested renaming (same as above #1).
>
> "contain_invalid_rfcolumn_walker" --> "contains_invalid_rfcolumn_walker"
> "contain_invalid_rfcolumn" --> "contains_invalid_rfcolumn"
>
> Also, update it in the comment for rf_context:
> +/*
> + * Information used to validate the columns in the row filter
> +expression. See
> + * contain_invalid_rfcolumn_walker for details.
> + */

I am not sure about the name because many existing
functions are named contain_xxx_xxx.
(for example contain_mutable_functions)

>
> 3. src/backend/commands/publicationcmds.c - bms
>
> + if (!rfisnull)
> + {
> + rf_context context = {0};
> + Node *rfnode;
> + Bitmapset *bms = NULL;
> +
> + context.pubviaroot = pub->pubviaroot;
> + context.parentid = publish_as_relid;
> + context.relid = relid;
> +
> + /*
> + * Remember columns that are part of the REPLICA IDENTITY. Note that
> + * REPLICA IDENTITY DEFAULT means primary key or nothing.
> + */
> + if (relation->rd_rel->relreplident == REPLICA_IDENTITY_DEFAULT) bms =
> + RelationGetIndexAttrBitmap(relation,
> + INDEX_ATTR_BITMAP_PRIMARY_KEY);
> + else if (relation->rd_rel->relreplident == REPLICA_IDENTITY_INDEX) bms
> + = RelationGetIndexAttrBitmap(relation,
> + INDEX_ATTR_BITMAP_IDENTITY_KEY);
> +
> + context.bms_replident = bms;
>
> There seems no need for a separate 'bms' variable here. Why not just assign
> directly to context.bms_replident like the code used to do?

Because I found it made the code exceed 80 cols, so I personally
think use a shorter variable could make it looks better.

>
> 4. src/backend/utils/cache/relcache.c - typo?
>
> /*
> - * If we know everything is replicated, there is no point to check for
> - * other publications.
> + * Check, if all columns referenced in the filter expression are part
> + * of the REPLICA IDENTITY index or not.
> + *
> + * If we already found the column in row filter which is not part of
> + * REPLICA IDENTITY index, skip the validation.
> */
>
> Shouldn't that comment say "already found a column" instead of "already found
> the column"?

Adjusted the comments here.

>
> 5. src/backend/replication/pgoutput/pgoutput.c - map member
>
> @@ -129,7 +169,7 @@ typedef struct RelationSyncEntry
> * same as 'relid' or if unnecessary due to partition and the ancestor
> * having identical TupleDesc.
> */
> - TupleConversionMap *map;
> + AttrMap *map;
> } RelationSyncEntry;
>
> I wondered if you should also rename this member to something more
> meaningful like 'attrmap' instead of just 'map'.
Changed.

Best regards,
Hou zj