bogus: logical replication rows/cols combinations

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

On Tue, Apr 26, 2022 at 4:00 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:

On 4/25/22 17:48, Alvaro Herrera wrote:

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?

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.

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.

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
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.

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.

I can take a stab at it, but it seems strange to not apply the same
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
this, as he wrote the row filter stuff.

I feel we can explain a bit more about this in docs. We already have
some explanation of how row filters are combined [1]https://www.postgresql.org/docs/devel/logical-replication-row-filter.html#LOGICAL-REPLICATION-ROW-FILTER-COMBINING. 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.

On Wed, Apr 27, 2022 at 10:25:50AM +0530, Amit Kapila wrote:

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.

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

On 2022-Apr-26, Tomas Vondra wrote:

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?

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.

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.

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

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).

I think we should do that, yeah.

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.

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)

On 2022-Apr-27, Amit Kapila wrote:

On Tue, Apr 26, 2022 at 4:00 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:

I can take a stab at it, but it seems strange to not apply the same
logic to evaluation of publish_as_relid.

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

Sorry, this argument makes no sense to me. The combination of both
features is not consistent, and both features are new.
'publish_as_relid' is an implementation detail. If the implementation
fails to follow the feature design, then the implementation must be
fixed ... not the design!

IMO, we should first determine how we want row filters and column lists
to work when used in conjunction -- for relations (sets of rows) in a
general sense. After we have done that, then we can use that design to
drive how we want partitioned tables to be handled for it. Keep in mind
that when users see a partitioned table, what they first see is a table.
They want all their tables to work in pretty much the same way --
partitioned or not partitioned. The fact that a table is partitioned
should affect as little as possible the way it interacts with other
features.

Now, another possibility is to say "naah, this is too hard", or even
"naah, there's no time to write all that for this release". That might
be okay, but in that case let's add an implementation restriction to
ensure that we don't paint ourselves in a corner regarding what is
reasonable behavior. For example, an easy restriction might be: if a
table is in multiple publications with mismatching row filters/column
lists, then a subscriber is not allowed to subscribe to both
publications. (Maybe this restriction isn't exactly what we need so
that it actually implements what we need, not sure). Then, if/when in
the future we implement this correctly, we can lift the restriction.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"La conclusión que podemos sacar de esos estudios es que
no podemos sacar ninguna conclusión de ellos" (Tanenbaum)

On Wednesday, April 27, 2022 12:56 PM From: Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Apr 26, 2022 at 4:00 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:

On 4/25/22 17:48, Alvaro Herrera wrote:

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?

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.

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.

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
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.

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.

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

On Wed, Apr 27, 2022 at 3:13 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2022-Apr-26, Tomas Vondra wrote:

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?

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.

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

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.

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

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).

I think we should do that, yeah.

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

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.

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.

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]/messages/by-id/OS0PR01MB5716B82315A067F1D78F247E94FA9@OS0PR01MB5716.jpnprd01.prod.outlook.com for the case you are talking about if the
user really needs something like that.

... 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.

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]: /messages/by-id/OS0PR01MB5716B82315A067F1D78F247E94FA9@OS0PR01MB5716.jpnprd01.prod.outlook.com

--
With Regards,
Amit Kapila.

On 2022-Apr-27, Amit Kapila wrote:

On Wed, Apr 27, 2022 at 3:13 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

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).

I think we should do that, yeah.

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

So we do things because they are easy and fast, rather than because they
work correctly?

... 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.

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.

OK, but this isn't relevant to what I wrote.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

On Wed, Apr 27, 2022 at 4:27 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2022-Apr-27, Amit Kapila wrote:

On Wed, Apr 27, 2022 at 3:13 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

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).

I think we should do that, yeah.

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

So we do things because they are easy and fast, rather than because they
work correctly?

The point is I am not sure if what you are saying is better behavior
than current but if others feel it is better then we can try to do
something for it. In the above sentence, I just wanted to say that it
will impact performance but if that is required then sure we should do
it that way.

--
With Regards,
Amit Kapila.

Hi,

so I've been looking at tweaking the code so that the behavior matches
Alvaro's expectations. It passes check-world but I'm not claiming it's
nowhere near commitable - the purpose is mostly to give better idea of
how invasive the change is etc.

As described earlier, this abandons the idea of building a single OR
expression from all the row filters (per action), and replaces that with
a list of per-publication info (struct PublicationInfo), combining info
about both row filters and column lists.

This means we can't initialize the row filters and column lists
separately, but at the same time. So pgoutput_row_filter_init was
modified to initialize both, and pgoutput_column_list_init was removed.

With this info, we can calculate column lists only for publications with
matching row filters, which is what the modified pgoutput_row_filter
does (the calculated column list is returned through a parameter).

This however does not remove the 'columns' from RelationSyncEntry
entirely. We still need that "superset" column list when sending schema.

Imagine two publications, one replicating (a,b) and the other (a,c),
maybe depending on row filter. send_relation_and_attrs() needs to send
info about all three attributes (a,b,c), i.e. about any attribute that
might end up being replicated.

We might try to be smarter and send the exact schema needed by the next
operation, i.e. when inserting (a,b) we'd make sure the last schema we
sent was (a,b) and invalidate/resend it otherwise. But that might easily
result in "trashing" where we send the schema and the next operation
invalidates it right away because it needs a different schema.

But there's another reason to do it like this - it seems desirable to
actually reset columns don't match the calculated column list. Using
Alvaro's example, it seems reasonable to expect these two transactions
to produce the same result on the subscriber:

1) insert (a,b) + update to (a,c)

insert into uno values (1, 2, 3);
update uno set a = -1 where a = 1;

2) insert (a,c)

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

But to do this, the update actually needs to send (-1,NULL,3).

So in this case we'll have (a,b,c) column list in RelationSyncEntry, and
only attributes on this list will be sent as part of schema. And DML
actions we'll calculate either (a,b) or (a,c) depending on the row
filter, and missing attributes will be replicated as NULL.

I haven't done any tests how this affect performance, but I have a
couple thoughts regarding that:

a) I kinda doubt the optimizations would really matter in practice,
because how likely is it that one relation is in many publications (in
the same subscription)?

b) Did anyone actually do some benchmarks that I could repeat, to see
how much worse this is?

c) AFAICS we could optimize this in at least some common cases. For
example we could combine the entries with matching row filters, and/or
column filters.

regards

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

On Thu, Apr 28, 2022 at 3:26 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:

so I've been looking at tweaking the code so that the behavior matches
Alvaro's expectations. It passes check-world but I'm not claiming it's
nowhere near commitable - the purpose is mostly to give better idea of
how invasive the change is etc.

I was just skimming through the patch and didn't find anything related
to initial sync handling. I feel the behavior should be same for
initial sync and replication.

--
With Regards,
Amit Kapila.

On 27.04.22 11:53, Alvaro Herrera wrote:

Now, another possibility is to say "naah, this is too hard", or even
"naah, there's no time to write all that for this release". That might
be okay, but in that case let's add an implementation restriction to
ensure that we don't paint ourselves in a corner regarding what is
reasonable behavior. For example, an easy restriction might be: if a
table is in multiple publications with mismatching row filters/column
lists, then a subscriber is not allowed to subscribe to both
publications. (Maybe this restriction isn't exactly what we need so
that it actually implements what we need, not sure). Then, if/when in
the future we implement this correctly, we can lift the restriction.

My feeling is also that we should prohibit the combinations that we
cannot make work correctly.

On 27.04.22 12:33, Amit Kapila wrote:

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.

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).

On 4/28/22 05:17, Amit Kapila wrote:

On Thu, Apr 28, 2022 at 3:26 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:

so I've been looking at tweaking the code so that the behavior matches
Alvaro's expectations. It passes check-world but I'm not claiming it's
nowhere near commitable - the purpose is mostly to give better idea of
how invasive the change is etc.

I was just skimming through the patch and didn't find anything related
to initial sync handling. I feel the behavior should be same for
initial sync and replication.

Yeah, sorry for not mentioning that - my goal was to explore and try
getting the behavior in regular replication right first, before
attempting to do the same thing in tablesync.

Attached is a patch doing the same thing in tablesync. The overall idea
is to generate copy statement with CASE expressions, applying filters to
individual columns. For Alvaro's example, this generates something like

SELECT
(CASE WHEN (a < 0) OR (a > 0) THEN a ELSE NULL END) AS a,
(CASE WHEN (a > 0) THEN b ELSE NULL END) AS b,
(CASE WHEN (a < 0) THEN c ELSE NULL END) AS c
FROM uno WHERE (a < 0) OR (a > 0)

And that seems to work fine. Similarly to regular replication we have to
use both the "total" column list (union of per-publication lists) and
per-publication (row filter + column list), but that's expected.

There's a couple options how we might optimize this for common cases.
For example if there's just a single publication, there's no need to
generate the CASE expressions - the WHERE filter will do the trick.

regards

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

On 4/28/22 14:26, Peter Eisentraut wrote:

On 27.04.22 12:33, Amit Kapila wrote:

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.

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).

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 table uno (a int primary key, b int, c int);

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

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

On Thu, Apr 28, 2022 at 11:00 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:

On 4/28/22 14:26, Peter Eisentraut wrote:

On 27.04.22 12:33, Amit Kapila wrote:

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).

That's a good question, actually. No, we don't combine the publications
like this, the row filters are kept "per action".

Right, and it won't work even if try to combine in this case because
of replica identity restrictions.

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 table uno (a int primary key, b int, c int);

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.

Right, but we don't know what previously would have happened maybe the
user would have altered the publication action after the initial row
is published in which case this DELETE is required as is shown in the
example below. We can only make the decision based on the current
tuple. For example:

create table uno (a int primary key, b int, c int);

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

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

-- create subscription for both these publications.

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

Alter publication dos set (publish='update');

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

Now, in this case, the old row was replicated and we would need a
DELETE corresponding to it.

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.

True, but that is what the combination of publications suggests. The
publication that publishes inserts have different criteria than
updates, so such behavior (a particular row when inserted will be
replicated but when it came as a result of an update it won't be
replicated) is expected.

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.

Yeah, this is the behavior of tablesync.c with or without rowfilter.
It ignores publication actions. So, if you update any tuple before
creation of subscription it will be replicated but the same update
won't be replicated after initial sync if the publication just
publishes 'insert'. I think we can't decide which data to copy based
on publication actions as COPY wouldn't know if a particular row is
due to a fresh insert or due to an update. In your example, it is
possible that row (-1, 3, 4) would have been there due to an update.

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?

True, I think to some extent we rely on users to define it sanely
otherwise currently also it can easily lead to even replication being
stuck. This can happen when the user is trying to operate on the same
table and define publication/subscription on multiple nodes for it.
See [1]https://commitfest.postgresql.org/38/3610/ where we trying to deal with such a problem.

[1]: https://commitfest.postgresql.org/38/3610/

--
With Regards,
Amit Kapila.

On Thu, Apr 28, 2022 at 5:56 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 27.04.22 12:33, Amit Kapila wrote:

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.

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.

So, what should be the behavior in the below cases:

Case-1:
pub1: "publish columns a and b if condition-X"
pub2: "publish column c if condition-X"

Isn't it okay to combine these?

Case-2:
pub1: "publish columns a and b if condition-X"
pub2: "publish columns c if condition-Y"

Here Y is subset of condition X (say something like condition-X: "col1

5" and condition-Y: "col1 > 10").

What should we do in such a case?

I think if there are some cases where combining them is okay but in
other cases, it is not okay then it is better to prohibit 'not-okay'
cases if that is feasible.

--
With Regards,
Amit Kapila.

On 4/29/22 06:48, Amit Kapila wrote:

On Thu, Apr 28, 2022 at 11:00 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:

On 4/28/22 14:26, Peter Eisentraut wrote:

On 27.04.22 12:33, Amit Kapila wrote:

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).

That's a good question, actually. No, we don't combine the publications
like this, the row filters are kept "per action".

Right, and it won't work even if try to combine in this case because
of replica identity restrictions.

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 table uno (a int primary key, b int, c int);

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.

Right, but we don't know what previously would have happened maybe the
user would have altered the publication action after the initial row
is published in which case this DELETE is required as is shown in the
example below. We can only make the decision based on the current
tuple. For example:

create table uno (a int primary key, b int, c int);

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

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

-- create subscription for both these publications.

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

Alter publication dos set (publish='update');

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

Now, in this case, the old row was replicated and we would need a
DELETE corresponding to it.

I think such issues due to ALTER of the publication are somewhat
expected, and I think users will understand they might need to resync
the subscription or something like that.

A similar example might be just changing the where condition,

create publication p for table t where (a > 10);

and then

alter publication p set table t where (a > 15);

If we replicated any rows with (a > 10) and (a <= 15), we'll just stop
replicating them. But if we re-create the subscription, we end up with a
different set of rows on the subscriber, omitting rows with (a <= 15).

In principle we'd need to replicate the ALTER somehow, to delete or
insert the rows that start/stop matching the row filter. It's a bit
similar to not replicating DDL, perhaps.

But I think the issue I've described is different, because you don't
have to change the subscriptions at all and you'll still have the
problem. I mean, imagine doing this:

-- publisher
create table t (a int primary key, b int);
create publication p for table t where (a > 10) with (publish='update');

-- subscriber
create table t (a int primary key, b int);
create subscription s connection '...' publication p;

-- publisher
insert into t select i, i from generate_series(1,20) s(i);
update t set b = b * 10;

-- subscriber
--> has no rows in "t"
--> recreate the subscription
drop subscription s;
create subscription s connection '...' publication p;

--> now it has all the rows with (a>10), because tablesync ignores
publication actions

The reason why I find this really annoying is that it makes it almost
impossible to setup two logical replicas that'd be "consistent", unless
you create them at the same time (= without any writes in between). And
it's damn difficult to think about the inconsistencies.

IMHO this all stems from allowing row filters and restricting pubactions
at the same time (notice this only used a single publication). So maybe
the best option would be to disallow combining these two features? That
would ensure the row filter filter is always applied to all actions in a
consistent manner, preventing all these issues.

Maybe that's not possible - maybe there are valid use cases that would
need such combination, and you mentioned replica identity might be an
issue (and maybe requiring RIF with row filters is not desirable).

So maybe we should at least warn against this in the documentation?

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.

True, but that is what the combination of publications suggests. The
publication that publishes inserts have different criteria than
updates, so such behavior (a particular row when inserted will be
replicated but when it came as a result of an update it won't be
replicated) is expected.

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.

Yeah, this is the behavior of tablesync.c with or without rowfilter.
It ignores publication actions. So, if you update any tuple before
creation of subscription it will be replicated but the same update
won't be replicated after initial sync if the publication just
publishes 'insert'. I think we can't decide which data to copy based
on publication actions as COPY wouldn't know if a particular row is
due to a fresh insert or due to an update. In your example, it is
possible that row (-1, 3, 4) would have been there due to an update.

Right. Which is why I think disallowing these two features (filtering
actions and row filters) might prevent this, because it eliminates this
ambiguity. It would not matter if a row was INSERTed or UPDATEd when
evaluating the row filter.

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?

True, I think to some extent we rely on users to define it sanely
otherwise currently also it can easily lead to even replication being
stuck. This can happen when the user is trying to operate on the same
table and define publication/subscription on multiple nodes for it.
See [1] where we trying to deal with such a problem.

[1] - https://commitfest.postgresql.org/38/3610/

That seems to deal with a circular replication, i.e. two logical
replication links - a bit like a multi-master. Not sure how is that
related to the issue we're discussing here?

regards

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

On Sat, Apr 30, 2022 at 2:02 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:

On 4/29/22 06:48, Amit Kapila wrote:

On Thu, Apr 28, 2022 at 11:00 PM Tomas Vondra

I think such issues due to ALTER of the publication are somewhat
expected, and I think users will understand they might need to resync
the subscription or something like that.

A similar example might be just changing the where condition,

create publication p for table t where (a > 10);

and then

alter publication p set table t where (a > 15);

If we replicated any rows with (a > 10) and (a <= 15), we'll just stop
replicating them. But if we re-create the subscription, we end up with a
different set of rows on the subscriber, omitting rows with (a <= 15).

In principle we'd need to replicate the ALTER somehow, to delete or
insert the rows that start/stop matching the row filter. It's a bit
similar to not replicating DDL, perhaps.

But I think the issue I've described is different, because you don't
have to change the subscriptions at all and you'll still have the
problem. I mean, imagine doing this:

-- publisher
create table t (a int primary key, b int);
create publication p for table t where (a > 10) with (publish='update');

-- subscriber
create table t (a int primary key, b int);
create subscription s connection '...' publication p;

-- publisher
insert into t select i, i from generate_series(1,20) s(i);
update t set b = b * 10;

-- subscriber
--> has no rows in "t"
--> recreate the subscription
drop subscription s;
create subscription s connection '...' publication p;

--> now it has all the rows with (a>10), because tablesync ignores
publication actions

The reason why I find this really annoying is that it makes it almost
impossible to setup two logical replicas that'd be "consistent", unless
you create them at the same time (= without any writes in between). And
it's damn difficult to think about the inconsistencies.

I understood your case related to the initial sync and it is with or
without rowfilter.

IMHO this all stems from allowing row filters and restricting pubactions
at the same time (notice this only used a single publication). So maybe
the best option would be to disallow combining these two features? That
would ensure the row filter filter is always applied to all actions in a
consistent manner, preventing all these issues.

Maybe that's not possible - maybe there are valid use cases that would
need such combination, and you mentioned replica identity might be an
issue

Yes, that is the reason we can't combine the row filters for all pubactions.

(and maybe requiring RIF with row filters is not desirable).

So maybe we should at least warn against this in the documentation?

Yeah, I find this as the most suitable thing to do to address your
concern. I would like to add this information to the 'Initial
Snapshot' page with some examples (both with and without a row
filter).

True, I think to some extent we rely on users to define it sanely
otherwise currently also it can easily lead to even replication being
stuck. This can happen when the user is trying to operate on the same
table and define publication/subscription on multiple nodes for it.
See [1] where we trying to deal with such a problem.

[1] - https://commitfest.postgresql.org/38/3610/

That seems to deal with a circular replication, i.e. two logical
replication links - a bit like a multi-master. Not sure how is that
related to the issue we're discussing here?

It is not directly related to what we are discussing here but I was
trying to emphasize the point that users need to define the logical
replication via pub/sub sanely otherwise they might see some weird
behaviors like that.

--
With Regards,
Amit Kapila.