RE: row filtering for logical replication

On Monday, January 24, 2022 4:38 PM Peter Smith <smithpb2250@gmail.com>
> 
> Thanks for all the patches!
> 
> Here are my review comments for v69-0001

Thanks for the comments!

> ~~~
> 
> 1. src/backend/executor/execReplication.c  CheckCmdReplicaIdentity call to
> RelationBuildPublicationDesc
> 
> + /*
> + * 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.
> + */
> + pubdesc = RelationBuildPublicationDesc(rel);
> 
> This code is leaky because never frees the palloc-ed memory for the pubdesc.
> 
> IMO change the RelationBuildPublicationDesc to pass in the
> PublicationDesc* from the call stack then can eliminate the palloc and risk of
> leaks.
> 
> ~~~
> 
> 2. src/include/utils/relcache.h - RelationBuildPublicationDesc
> 
> +struct PublicationDesc;
> +extern struct PublicationDesc *RelationBuildPublicationDesc(Relation
> +relation);
> 
> (Same as the previous comment #1). Suggest to change the function signature
> to be void and pass the PublicationDesc* from stack instead of palloc-ing it
> within the function

I agree with these changes and Greg has posted a separate patch[1] to change
these. I think it might be better to change these after that separate patch get
committed because some discussions are still going on in that thread.

[1] https://postgr.es/m/CAJcOf-d0%3DvQx1Pzbf%2BLVarywejJFS5W%2BM6uR%2B2d0oeEJ2VQ%2BEw%40mail.gmail.com

> 
> 3. src/backend/utils/cache/relcache.c - RelationBuildPublicationDesc
> 
> +RelationBuildPublicationDesc(Relation relation)
>  {
>   List    *puboids;
>   ListCell   *lc;
>   MemoryContext oldcxt;
>   Oid schemaid;
> - PublicationActions *pubactions = palloc0(sizeof(PublicationActions));
> + List    *ancestors = NIL;
> + Oid relid = RelationGetRelid(relation); AttrNumber invalid_rfcolnum =
> + InvalidAttrNumber; PublicationDesc *pubdesc =
> + palloc0(sizeof(PublicationDesc)); PublicationActions *pubactions =
> + &pubdesc->pubactions;
> +
> + pubdesc->rf_valid_for_update = true;
> + pubdesc->rf_valid_for_delete = true;
> 
> IMO it wold be better to change the "sense" of those variables.
> e.g.
> 
> "rf_valid_for_update" --> "rf_invalid_for_update"
> "rf_valid_for_delete" --> "rf_invalid_for_delete"
> 
> That way they have the same 'sense' as the AttrNumbers so it all reads better to
> me.
> 
> Also, it means no special assignment is needed because the palloc0 will set
> them correctly

Since Alvaro also had some comments about the cached things and the discussion
is still going on, I will note down this comment and change it later.

> 4. src/backend/utils/cache/relcache.c - RelationBuildPublicationDesc
> 
> - if (relation->rd_pubactions)
> + if (relation->rd_pubdesc)
>   {
> - pfree(relation->rd_pubactions);
> - relation->rd_pubactions = NULL;
> + pfree(relation->rd_pubdesc);
> + relation->rd_pubdesc = NULL;
>   }
> 
> What is the purpose of this code? Can't it all just be removed?
> e.g. Can't you Assert that relation->rd_pubdesc is NULL at this point?
> 
> (if it was not-null the function would have returned immediately from the top)

I think it might be better to change this as a separate patch.

> 5. src/include/catalog/pg_publication.h - typedef struct PublicationDesc
> 
> +typedef struct PublicationDesc
> +{
> + /*
> + * true if the columns referenced in row filters which are used for
> +UPDATE
> + * or DELETE are part of the replica identity, or the publication
> +actions
> + * do not include UPDATE or DELETE.
> + */
> + bool rf_valid_for_update;
> + bool rf_valid_for_delete;
> +
> + AttrNumber invalid_rfcol_update;
> + AttrNumber invalid_rfcol_delete;
> +
> + PublicationActions pubactions;
> +} PublicationDesc;
> +
> 
> I did not see any point really for the pairs of booleans and AttNumbers.
> AFAIK both of them shared exactly the same validation logic so I think you can
> get by using fewer members here.

the pairs of booleans are intended to fix the problem[2] reported earlier.
[2]
https://www.postgresql.org/message-id/OS0PR01MB611367BB85115707CDB2F40CFB5A9%40OS0PR01MB6113.jpnprd01.prod.outlook.com
> 
> 6. src/tools/pgindent/typedefs.list
> 
> Missing the new typedef PublicationDesc

Added.

Attach the V70 patch set which fixed above comments and Greg's comments[3].

[3] https://www.postgresql.org/message-id/CAJcOf-eUnXPSDR1smg9VFktr6OY5%3D8zAsCX-rqctBdfgoEavDA%40mail.gmail.com

Best regards,
Hou zj