Re: row filtering for logical replication

On Tue, Dec 21, 2021 at 5:58 AM Euler Taveira <euler(at)eulerto(dot)com> wrote:
>
> On Mon, Dec 20, 2021, at 12:10 AM, houzj(dot)fnst(at)fujitsu(dot)com wrote:
>
> Attach the V49 patch set, which addressed all the above comments on the 0002
> patch.
>
> I've been testing the latest versions of this patch set. I'm attaching a new
> patch set based on v49. The suggested fixes are in separate patches after the
> current one so it is easier to integrate them into the related patch. The
> majority of these changes explains some decision to improve readability IMO.
>
> row-filter x row filter. I'm not a native speaker but "row filter" is widely
> used in similar contexts so I suggest to use it. (I didn't adjust the commit
> messages)

Fixed in v51* [1]. And I also updated the commit comments.

>
> An ancient patch use the term coerce but it was changed to cast. Coercion
> implies an implicit conversion [1]. If you look at a few lines above you will
> see that this expression expects an implicit conversion.

Fixed in v51* [1]

>
> I modified the query to obtain the row filter expressions to (i) add the schema
> pg_catalog to some objects and (ii) use NOT EXISTS instead of subquery (it
> reads better IMO).

Not changed in v51, but IIUC this might be fixed soon if it is
confirmed to be better. [2 Amit]

> A detail message requires you to capitalize the first word of sentences and
> includes a period at the end.

Fixed in v51* [1]

>
> It seems all server messages and documentation use the terminology "WHERE
> clause". Let's adopt it instead of "row filter".

Fixed in v51* [1]

>
> I reviewed 0003. It uses TupleTableSlot instead of HeapTuple. I probably missed
> the explanation but it requires more changes (logicalrep_write_tuple and 3 new
> entries into RelationSyncEntry). I replaced this patch with a slightly
> different one (0005 in this patch set) that uses HeapTuple instead. I didn't
> only simple tests and it requires tests. I noticed that this patch does not
> include a test to cover the case where TOASTed values are not included in the
> new tuple. We should probably add one.

Not changed in v51. See response from Ajin [3 Ajin].

>
> I agree with Amit that it is a good idea to merge 0001, 0002, and 0005. I would
> probably merge 0004 because it is just isolated changes.

Fixed in v51* [1] per Amit's suggestion (so the 0004 is still separate)

------
[1] https://www.postgresql.org/message-id/CAHut%2BPs%2BdACvefCZasRE%3DP%3DDtaNmQvM3kiGyKyBHANA0yGcTZw%40mail.gmail.com
[2 Amit] https://www.postgresql.org/message-id/CAA4eK1KwoA5k8v9z9e4ZPN_X%3D1GAmQmsWyauFwZpKiSHqy6eZA%40mail.gmail.com
[3 Ajin] https://www.postgresql.org/message-id/CAFPTHDbfpPNh3GLGjySS%3DAuRfbQPQFNvfiyG1GDQW2kz1yT7Og%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia