RE: row filtering for logical replication - Mailing list pgsql-hackers

From houzj.fnst@fujitsu.com
Subject RE: row filtering for logical replication
Date
Msg-id OS0PR01MB5716083FCF337DA265A62C6994209@OS0PR01MB5716.jpnprd01.prod.outlook.com
Whole thread Raw
In response to Re: row filtering for logical replication  (Peter Smith <smithpb2250@gmail.com>)
Responses Re: row filtering for logical replication
List pgsql-hackers
On Monday, January 24, 2022 4:38 PM Peter Smith <smithpb2250@gmail.com> wrote:
> 
> Thanks for all the patches!
> 
> Here are my review comments for v69-0001
> 
> ~~~
> 
> 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

Changed in V71.

> 
> 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

Think again, I am not sure it's better to have an invalid_... flag.
It seems more natural to have a valid_... flag.

Best regards,
Hou zj

pgsql-hackers by date:

Previous
From: Masahiko Sawada
Date:
Subject: Re: Skipping logical replication transactions on subscriber side
Next
From: "houzj.fnst@fujitsu.com"
Date:
Subject: RE: row filtering for logical replication