Re: row filtering for logical replication

On Tue, Dec 7, 2021 at 5:36 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> We were mid-way putting together the next v45* when your latest
> attachment was posted over the weekend. So we will proceed with our
> original plan to post our v45* (tomorrow).
>
> After v45* is posted we will pause to find what are all the
> differences between your unified patch and our v45* patch set. Our
> intention is to integrate as many improvements as possible from your
> changes into the v46* etc that will follow tomorrow’s v45*. On some
> points, we will most likely need further discussion.

Posting an update for review comments, using contributions majorly
from Peter Smith.
I've also included changes based on Euler's combined patch, specially
changes to documentation and test cases.
I have left out Hou-san's 0005, in this patch-set. Hou-san will
provide a rebased update based on this.

This patch addresses the following review comments:

On Wed, Nov 24, 2021 at 8:52 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> Few comments:
> 1) I'm not sure if we will be able to throw a better error message in
> this case "ERROR: missing FROM-clause entry for table "t4"", if
> possible you could change it.

Fixed this.

On Thu, Dec 2, 2021 at 7:40 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> 1) Both testpub5a and testpub5c publication are same, one of them can be removed
> +SET client_min_messages = 'ERROR';
> +CREATE PUBLICATION testpub5a FOR TABLE testpub_rf_tbl1 WHERE (a > 1)
> WITH (publish="insert");
> +CREATE PUBLICATION testpub5b FOR TABLE testpub_rf_tbl1;
> +CREATE PUBLICATION testpub5c FOR TABLE testpub_rf_tbl1 WHERE (a > 3)
> WITH (publish="insert");
> +RESET client_min_messages;
> +\d+ testpub_rf_tbl1
> +DROP PUBLICATION testpub5a, testpub5b, testpub5c;

Fixed

On Fri, Dec 3, 2021 at 6:16 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
>
>
> doc/src/sgml/ref/create_subscription.sgml
> (2) Refer to Notes
>
> Perhaps a link to the Notes section should be used here, as follows:
>
> - copied. Refer to the Notes section below.
> + copied. Refer to the <xref
> linkend="sql-createsubscription-notes"/> section below.

Fixed

> 1) Typo in patch comment
> "Specifially"

Fixed

> src/backend/catalog/pg_publication.c
> 2) bms_replident comment
> Member "Bitmapset *bms_replident;" in rf_context should have a
> comment, maybe something like "set of replica identity col indexes".
>

Fixed

> 3) errdetail message
> In rowfilter_walker(), the "forbidden" errdetail message is loaded
> using gettext() in one instance, but just a raw formatted string in
> other cases. Shouldn't they all consistently be translated strings?
>

Fixed

> (i)
> if (slot == NULL || TTS_EMPTY(slot))
> can be replaced with:
> if (TupIsNull(slot))
>

Fixed

> (ii) In the above case (where values and nulls are palloc'd),
> shouldn't the values and nulls be pfree()d at the end of the function?
>
>

Fixed, changed it into fixed arrays

On Thu, Dec 2, 2021 at 2:32 PM tanghy(dot)fnst(at)fujitsu(dot)com
<tanghy(dot)fnst(at)fujitsu(dot)com> wrote:
>>
> 1. This is an example in publication doc, but in fact it's not allowed. Should we
> change this example?
>

Fixed.

> 2. A typo in 0002 patch.
>
> + * drops such a user-defnition or if there is any other error via its function,
>
> "user-defnition" should be "user-definition".

Fixed

On Fri, Dec 3, 2021 at 12:59 AM Euler Taveira <euler(at)eulerto(dot)com> wrote:
>
> ExprState cache logic is basically all the same as before (including
> all the OR combining), but there are now 4x ExprState caches keyed and
> separated by the 4x different pubactions.
>
> row filter is not applied for TRUNCATEs so it is just 3 operations.
>

Fixed

regards,
Ajin Cherian
Fujitsu Australia