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