RE: row filtering for logical replication

On Tues, Jan 18, 2022 8:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Mon, Jan 17, 2022 at 8:58 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > Attach the V66 patch set which addressed Amit, Peter and Greg's comments.
> >
> 
> Thanks, some more comments, and suggestions:
> 
> 1.
> /*
> + * If no record in publication, check if the table is the partition
> + * of a published partitioned table. If so, the table has no row
> + * filter.
> + */
> + else if (!pub->pubviaroot)
> + {
> + List    *schemarelids;
> + List    *relids;
> +
> + schemarelids = GetAllSchemaPublicationRelations(pub->oid,
> + PUBLICATION_PART_LEAF);
> + relids = GetPublicationRelations(pub->oid,
> + PUBLICATION_PART_LEAF);
> +
> + if (list_member_oid(schemarelids, entry->publish_as_relid) ||
> + list_member_oid(relids, entry->publish_as_relid))
> + pub_no_filter = true;
> +
> + list_free(schemarelids);
> + list_free(relids);
> +
> + if (!pub_no_filter)
> + continue;
> + }
> 
> As far as I understand this handling is required only for partition
> tables but it seems to be invoked for non-partition tables as well.
> Please move the comment inside else if block and expand a bit more to
> say why it is necessary to not directly set pub_no_filter here.

Changed.

> Note,
> that I think this can be improved (avoid cache lookups) if we maintain
> a list of pubids in relsyncentry but I am not sure that is required
> because this is a rare case and needs to be done only one time.

I will do some research about this.

> 2.
>  static HTAB *OpClassCache = NULL;
> 
> -
>  /* non-export function prototypes */
> 
> Spurious line removal. I have added back in the attached top-up patch.
> 
> Apart from the above, I have made some modifications to other comments.

Thanks for the changes and comments.

Attach the V67 patch set which address the above comments.

The new version patch also includes:
- Some code comments update suggested by Peter [1] and Greg [2]
- Move the initialization of cached slot into a separate function because we now
  use the cached slot even if there is no filter.
- Remove an unused parameter in pgoutput_row_filter_init.
- Improve the memory context initialization of row filter.
- Fix some tab-complete bugs (fix provided by Peter)

[1] https://www.postgresql.org/message-id/CAHut%2BPtPVqXVsqBHU3wTppU_cK5xuS7TkqT1XJLJmn%2BTpt905w%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAJcOf-eWhCtdKXc9_5JASJ1sU0nGOSp%2B2nzLk01O2%3DZy7v1ApQ%40mail.gmail.com

Best regards,
Hou zj