Re: row filtering for logical replication

On Tue, Dec 21, 2021 at 12:28 AM Euler Taveira <euler@eulerto.com> wrote:
>
> On Mon, Dec 20, 2021, at 12:10 AM, houzj.fnst@fujitsu.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)
>
> 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.
>
> 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).
>

Yeah, I think that reads better, but maybe we can once check the plan
of both queries and see if there is any significant difference between
one of those.

> A detail message requires you to capitalize the first word of sentences and
> includes a period at the end.
>
> It seems all server messages and documentation use the terminology "WHERE
> clause". Let's adopt it instead of "row filter".
>
> 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.
>

Yeah, it would be good to add such a test.

-- 
With Regards,
Amit Kapila.