RE: row filtering for logical replication

On Thursday, January 13, 2022 6:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Jan 12, 2022 at 7:19 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Wed, Jan 12, 2022 5:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > Attach the v63 patch set which include the following changes.
> >

Thanks for the comments !

> Few comments:
> =============
> 1.
> +
> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +      <structfield>prqual</structfield> <type>pg_node_tree</type>
> +      </para>
> +      <para>Expression tree (in <function>nodeToString()</function>
> +      representation) for the relation's qualifying condition</para></entry>
> +     </row>
> 
> Let's slightly modify this as: "Expression tree (in
> <function>nodeToString()</function> representation) for the relation's
> qualifying condition. Null if there is no qualifying condition."

Changed.

> 2.
> +   A <literal>WHERE</literal> clause allows simple expressions. The simple
> +   expression cannot contain any aggregate or window functions,
> non-immutable
> +   functions, user-defined types, operators or functions.
> 
> This part in the docs should be updated to say something similar to what we
> have in the commit message for this part or maybe additionally in some way we
> can say which other forms of expressions are not allowed.

Temporally used the description in commit message.

> 3.
> +   for which the <replaceable
> class="parameter">expression</replaceable> returns
> +   false or null will not be published.
> +   If the subscription has several publications in which
> +   the same table has been published with different
> + <literal>WHERE</literal>
> 
> In the above text line spacing appears a bit odd to me. There doesn't seem to be
> a need for extra space after line-2 and line-3 in above-quoted text.

I adjusted these text lines.

> 4.
> /*
> + * Return the relid of the topmost ancestor that is published via this
> 
> We normally seem to use Returns in similar places.

Changed

> 
> 6.
> +static void
> +transformPubWhereClauses(List *tables, const char *queryString)
> 
> To keep the function naming similar to other nearby functions, it is better to
> name this as TransformPubWhereClauses.

Changed.

> 7. In AlterPublicationTables(), won't it better if we
> transformPubWhereClauses() after
> CheckObjSchemaNotAlreadyInPublication() to avoid extra processing in case of
> errors.

Changed.

> 8.
> + /*
> + * Check if the relation is member of the existing schema in the
> + * publication or member of the schema list specified.
> + */
>   CheckObjSchemaNotAlreadyInPublication(rels, schemaidlist,
>     PUBLICATIONOBJ_TABLE);
> 
> I don't see the above comment addition has anything to do with this patch. Can
> we remove it?

Removed.

> 9.
>  CheckCmdReplicaIdentity(Relation rel, CmdType cmd)  {
>   PublicationActions *pubactions;
> + AttrNumber bad_rfcolnum;
> 
>   /* We only need to do checks for UPDATE and DELETE. */
>   if (cmd != CMD_UPDATE && cmd != CMD_DELETE)
>   return;
> 
> + if (rel->rd_rel->relreplident == REPLICA_IDENTITY_FULL) return;
> +
> + /*
> + * It is only safe to execute UPDATE/DELETE when all columns referenced
> + in
> + * the row filters from publications which the relation is in are valid
> + -
> + * i.e. when all referenced columns are part of REPLICA IDENTITY, or
> + the
> + * table does not publish UPDATES or DELETES.
> + */
> + bad_rfcolnum = GetRelationPublicationInfo(rel, true);
> 
> Can we name this variable as invalid_rf_column?
Changed.

Attach the V64 patch set which addressed Alvaro, Amit and Peter's comments.

The new version patch also include some other changes:
- Fix a table sync bug[1] by using the SQL suggested by Tang[1]
- Adjust the row filter initialize code related to FOR ALL TABLE IN SCHEMA to
  make sure it gets the correct row filter.
- Update the documents.
- Rebased the patch based on recent commit 025b92

[1]
https://www.postgresql.org/message-id/OS0PR01MB6113BB510435B16E9F0B2A59FB519%40OS0PR01MB6113.jpnprd01.prod.outlook.com

Best regards,
Hou zj