Subject: Re: bogus: logical replication rows/cols combinations

From: Tomas Vondra <[email-masked]>
Date: Tue, 26 Apr 2022 00:30:21 +0200

On 4/25/22 17:48, Alvaro Herrera wrote:
> I just noticed that publishing tables on multiple publications with
> different row filters and column lists has somewhat surprising behavior.
> To wit: if a column is published in any row-filtered publication, then
> the values for that column are sent to the subscriber even for rows that
> don't match the row filter, as long as the row matches the row filter
> for any other publication, even if that other publication doesn't
> include the column.
> 
> Here's an example.
> 
> Publisher:
> 
> create table uno (a int primary key, b int, c int);
> create publication uno for table uno (a, b) where (a > 0);
> create publication dos for table uno (a, c) where (a < 0);
> 
> Here, we specify: publish columns a,b for rows with positive a, and
> publish columns a,c for rows with negative a.
> 
> What happened next will surprise you!  Well, maybe not.  On subscriber:
> 
> create table uno (a int primary key, b int, c int);
> create subscription sub_uno connection 'port=55432 dbname=alvherre' publication uno,dos;
> 
> Publisher:
> insert into uno values (1, 2, 3), (-1, 3, 4);
> 
> Publication 'uno' only has columns a and b, so row with a=1 should not
> have value c=3.  And publication 'dos' only has columns a and c, so row
> with a=-1 should not have value b=3.  But, on subscriber:
> 
> table uno;
>  a  │ b │ c 
> ────┼───┼───
>   1 │ 2 │ 3
>  -1 │ 3 │ 4
> 
> q.e.d.
> 
> I think results from a too simplistic view on how to mix multiple
> publications with row filters and column lists.  IIRC we are saying "if
> column X appears in *any* publication, then the value is published",
> period, and don't stop to evaluate the row filter corresponding to each
> of those publications. 
> 

Right.

> The desired result on subscriber is:
> 
> table uno;
>  a  │ b │ c 
> ────┼───┼───
>   1 │ 2 │
>  -1 │   │ 4
> 
> 
> Thoughts?
> 

I'm not quite sure which of the two behaviors is more "desirable". In a
way, it's somewhat similar to publish_as_relid, which is also calculated
not considering which of the row filters match?

But maybe you're right and it should behave the way you propose ... the
example I have in mind is a use case replicating table with two types of
rows - sensitive and non-sensitive. For sensitive, we replicate only
some of the columns, for non-sensitive we replicate everything. Which
could be implemented as two publications

create publication sensitive_rows
   for table t (a, b) where (is_sensitive);

create publication non_sensitive_rows
   for table t where (not is_sensitive);

But the way it's implemented now, we'll always replicate all columns,
because the second publication has no column list.

Changing this to behave the way you expect would be quite difficult,
because at the moment we build a single OR expression from all the row
filters. We'd have to keep the individual expressions, so that we can
build a column list for each of them (in order to ignore those that
don't match).

We'd have to remove various other optimizations - for example we can't
just discard row filters if we found "no_filter" publication. Or more
precisely, we'd have to consider column lists too.

In other words, we'd have to merge pgoutput_column_list_init into
pgoutput_row_filter_init, and then modify pgoutput_row_filter to
evaluate the row filters one by one, and build the column list.

I can take a stab at it, but it seems strange to not apply the same
logic to evaluation of publish_as_relid. I wonder what Amit thinks about
this, as he wrote the row filter stuff.

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Amit Kapila <[email-masked]>
Date: Wed, 27 Apr 2022 10:25:50 +0530

> not considering which of the row filters match?

Right, or in other words, we check all publications to decide it and
similar is the case for publication actions which are also computed
independently for all publications.

> just discard row filters if we found "no_filter" publication.
>

I don't think that is the right way. We need some way to combine
expressions and I feel the current behavior is sane. I mean to say
that even if there is one publication that has no filter (column/row),
we should publish all rows with all columns. Now, as mentioned above
combining row filters or column lists for all publications appears to
be consistent with what we already do and seems correct behavior to
me.

To me, it appears that the method used to decide whether a particular
table is published or not is also similar to what we do for row
filters or column lists. Even if there is one publication that
publishes all tables, we consider the current table to be published
irrespective of whether other publications have published that table
or not.

> Or more

Hmm, I think even if we want to do something here, we also need to
think about how to achieve similar behavior for initial tablesync
which will be more tricky.

> logic to evaluation of publish_as_relid.
>

Yeah, the current behavior seems to be consistent with what we already do.

> I wonder what Amit thinks about

I feel we can explain a bit more about this in docs. We already have
some explanation of how row filters are combined [1]. We can probably
add a few examples for column lists.

[1] - https://www.postgresql.org/docs/devel/logical-replication-row-filter.html#LOGICAL-REPLICATION-ROW-FILTER-COMBINING

-- 
With Regards,
Amit Kapila.

---

From: Michael Paquier <[email-masked]>
Date: Wed, 27 Apr 2022 15:12:35 +0900

> add a few examples for column lists.

I am not completely sure exactly what we should do here, but this
stuff needs to be at least discussed.  I have added an open item.
--
Michael

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 27 Apr 2022 11:43:16 +0200

> not considering which of the row filters match?

I grepped doc/src/sgml for `publish_as_relid` and found no hits, so
I suppose it's not a user-visible feature as such.

> some of the columns, for non-sensitive we replicate everything.

Exactly.  If we blindly publish row/column values that aren't in *any*
publications, this may lead to leaking protected values.

> build a column list for each of them (in order to ignore those that

I think we should do that, yeah.

> this, as he wrote the row filter stuff.

By grepping publicationcmds.c, it seems that publish_as_relid refers to
the ancestor partitioned table that is used for column list and
rowfilter determination, when a partition is being published as part of
it.  I don't think these things are exactly parallel.

... In fact I think they are quite orthogonal: probably you should be
able to publish a partitioned table in two publications, with different
rowfilters and different column lists (which can come from the
topmost partitioned table), and each partition should still work in the
way I describe above.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"[PostgreSQL] is a great group; in my opinion it is THE best open source
development communities in existence anywhere."                (Lamar Owen)

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 27 Apr 2022 10:08:12 +0000

> which will be more tricky.

I think it could be difficult to make the initial tablesync behave the same.
Currently, we make a "COPY" command to do the table sync, I am not sure
how to change the "COPY" query to achieve the expected behavior here.

BTW, For the use case mentioned here:
"""
replicating table with two types of
rows - sensitive and non-sensitive. For sensitive, we replicate only
some of the columns, for non-sensitive we replicate everything.
""" 

One approach to do this is to create two subscriptions and two
publications which seems a workaround.
-----
create publication uno for table uno (a, b) where (a > 0);
create publication dos for table uno (a, c) where (a < 0);

create subscription sub_uno connection 'port=55432 dbname=alvherre' publication uno;
create subscription sub_dos connection 'port=55432 dbname=alvherre' publication dos;
-----

Best regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Wed, 27 Apr 2022 16:03:11 +0530

> I suppose it's not a user-visible feature as such.

`publish_as_relid` is computed based on 'publish_via_partition_root'
setting of publication which is a user-visible feature.

> I think we should do that, yeah.

This can hit the performance as we need to evaluate each expression
for each row.

> it.

Yeah, this is true when the corresponding publication has set
'publish_via_partition_root' as true.

>  I don't think these things are exactly parallel.
>

Currently, when the subscription has multiple publications, we combine
the objects, and actions of those publications. It happens for
'publish_via_partition_root', publication actions, tables, column
lists, or row filters. I think the whole design works on this idea
even the initial table sync. I think it might need a major change
(which I am not sure about at this stage) if we want to make the
initial sync also behave similar to what you are proposing.

I feel it would be much easier to create two different subscriptions
as mentioned by Hou-San [1] for the case you are talking about if the
user really needs something like that.

> topmost partitioned table), and each partition should still work in the

We consider the column lists or row filters for either the partition
(on which the current operation is performed) or partitioned table
based on 'publish_via_partition_root' parameter of publication.

[1] - https://www.postgresql.org/message-id/OS0PR01MB5716B82315A067F1D78F247E94FA9%40OS0PR01MB5716.jpnprd01.prod.outlook.com

-- 
With Regards,
Amit Kapila.

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 28 Apr 2022 14:26:25 +0200

> initial sync also behave similar to what you are proposing.

If one publication says "publish if insert" and another publication says 
"publish if update", then the combination of that is clearly "publish if 
insert or update".  Similarly, if one publication says "WHERE (foo)" and 
one says "WHERE (bar)", then the combination is "WHERE (foo OR bar)".

But if one publication says "publish columns a and b if condition-X" and 
another publication says "publish columns a and c if not-condition-X", 
then the combination is clearly *not* "publish columns a, b, c if true". 
  That is not logical, in the literal sense of that word.

I wonder how we handle the combination of

pub1: publish=insert WHERE (foo)
pub2: publish=update WHERE (bar)

I think it would be incorrect if the combination is

pub1, pub2: publish=insert,update WHERE (foo OR bar).

---

From: Tomas Vondra <[email-masked]>
Date: Thu, 28 Apr 2022 19:30:08 +0200

> pub1, pub2: publish=insert,update WHERE (foo OR bar).

That's a good question, actually. No, we don't combine the publications
like this, the row filters are kept "per action". But the exact behavior
turns out to be rather confusing in this case.

(Note: This has nothing to do with column lists.)

Consider an example similar to what Alvaro posted earlier:

  create publication uno for table uno where (a > 0)
    with (publish='insert');

  create publication dos for table uno where (a < 0)
    with (publish='update');

And do this:

  insert into uno values (1, 2, 3), (-1, 3, 4)

which on the subscriber produces just one row, because (a<0) replicates
only updates:

   a | b | c
  ---+---+---
   1 | 2 | 3
  (1 row)

Now, let's update the (a<0) row.

  update uno set a = 2 where a = -1;

It might seem reasonable to expect the updated row (2,3,4) to appear on
the subscriber, but no - that's not what happens. Because we have (a<0)
for UPDATE, and we evaluate this on the old row (matches) and new row
(does not match). And pgoutput_row_filter() decides the update needs to
be converted to DELETE, despite the old row was not replicated at all.

I'm not sure if pgoutput_row_filter() can even make reasonable decisions
with such configuration (combination of row filters, actions ...). But
it sure seems confusing, because if you just inserted the updated row,
it would get replicated.

Which brings me to a second problem, related to this one. Imagine you
create the subscription *after* inserting the two rows. In that case you
get this:

   a  | b | c
  ----+---+---
    1 | 2 | 3
   -1 | 3 | 4
  (2 rows)

because tablesync.c ignores which actions is the publication (and thus
the rowfilter) defined for.

I think it's natural to expect that (INSERT + sync) and (sync + INSERT)
produce the same output on the subscriber.

I'm not sure we can actually make this perfectly sane with arbitrary
combinations of filters and actions. It would probably depend on whether
the actions are commutative, associative and stuff like that. But maybe
we can come up with restrictions that'd make this sane?

regards