Re: row filtering for logical replication

Hi,

I finally had time to take a closer look at the patch again, so here's 
some review comments. The thread is moving fast, so chances are some of 
the comments are obsolete or were already raised in the past.


1) I wonder if we should use WHERE or WHEN to specify the expression. 
WHERE is not wrong, but WHEN (as used in triggers) might be better.


2) create_publication.sgml says:

    A <literal>NULL</literal> value causes the expression to evaluate
    to false; avoid using columns without not-null constraints in the
    <literal>WHERE</literal> clause.

That's not quite correct, I think - doesn't the expression evaluate to 
NULL (which is not TRUE, so it counts as mismatch)?

I suspect this whole paragraph (talking about NULL in old/new rows) 
might be a bit too detailed / low-level for user docs.


3) create_subscription.sgml

     <literal>WHERE</literal> clauses, rows must satisfy all expressions
     to be copied. If the subscriber is a

I'm rather skeptical about the principle that all expressions have to 
match - I'd have expected exactly the opposite behavior, actually.

I see a subscription as "a union of all publications". Imagine for 
example you have a data set for all customers, and you create a 
publication for different parts of the world, like

   CREATE PUBLICATION customers_france
      FOR TABLE customers WHERE (country = 'France');

   CREATE PUBLICATION customers_germany
      FOR TABLE customers WHERE (country = 'Germany');

   CREATE PUBLICATION customers_usa
      FOR TABLE customers WHERE (country = 'USA');

and now you want to subscribe to multiple publications, because you want 
to replicate data for multiple countries (e.g. you want EU countries). 
But if you do

   CREATE SUBSCRIPTION customers_eu
          PUBLICATION customers_france, customers_germany;

then you won't get anything, because each customer belongs to just a 
single country. Yes, I could create multiple individual subscriptions, 
one for each country, but that's inefficient and may have a different 
set of issues (e.g. keeping them in sync when a customer moves between 
countries).

I might have missed something, but I haven't found any explanation why 
the requirement to satisfy all expressions is the right choice.

IMHO this should be 'satisfies at least one expression' i.e. we should 
connect the expressions by OR, not AND.


4) pg_publication.c

It's a bit suspicious we're adding includes for parser to a place where 
there were none before. I wonder if this might indicate some layering 
issue, i.e. doing something in the wrong place ...


5) publicationcmds.c

I mentioned this in my last review [1] already, but I really dislike the 
fact that OpenTableList accepts a list containing one of two entirely 
separate node types (PublicationTable or Relation). It was modified to 
use IsA() instead of a flag, but I still find it ugly, confusing and 
possibly error-prone.

Also, not sure mentioning the two different callers explicitly in the 
OpenTableList comment is a great idea - it's likely to get stale if 
someone adds another caller.


6) parse_oper.c

I'm having some second thoughts about (not) allowing UDFs ...

Yes, I get that if the function starts failing, e.g. because querying a 
dropped table or something, that breaks the replication and can't be 
fixed without a resync.

That's pretty annoying, but maybe disallowing anything user-defined 
(functions and operators) is maybe overly anxious? Also, extensibility 
is one of the hallmarks of Postgres, and disallowing all custom UDF and 
operators seems to contradict that ...

Perhaps just explaining that the expression can / can't do in the docs, 
with clear warnings of the risks, would be acceptable.


7) exprstate_list

I'd just call the field / variable "exprstates", without indicating the 
data type. I don't think we do that anywhere.


8) RfCol

Do we actually need this struct? Why not to track just name or attnum, 
and lookup the other value in syscache when needed?


9)  rowfilter_expr_checker

    * Walk the parse-tree to decide if the row-filter is valid or not.

I don't see any clear explanation what does "valid" mean.


10) WHERE expression vs. data type

Seem ATExecAlterColumnType might need some changes, because changing a 
data type for column referenced by the expression triggers this:

   test=# alter table t alter COLUMN c type text;
   ERROR:  unexpected object depending on column: publication of
           table t in publication p


11) extra (unnecessary) parens in the deparsed expression

test=# alter publication p add table t where ((b < 100) and (c < 100));
ALTER PUBLICATION
test=# \dRp+ p
                               Publication p
  Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root
-------+------------+---------+---------+---------+-----------+----------
  user  | f          | t       | t       | t       | t         | f
Tables:
     "public.t" WHERE (((b < 100) AND (c < 100)))


12) WHERE expression vs. changing replica identity

Peter Smith already mentioned this in [3], but there's a bunch of places 
that need to check the expression vs. replica identity. Consider for 
example this:

test=# alter publication p add table t where (b < 100);
ERROR:  cannot add relation "t" to publication
DETAIL:  Row filter column "b" is not part of the REPLICA IDENTITY

test=# alter table t replica identity full;
ALTER TABLE

test=# alter publication p add table t where (b < 100);
ALTER PUBLICATION

test=# alter table t replica identity using INDEX t_pkey ;
ALTER TABLE

Which means the expression is not covered by the replica identity.


12) misuse of REPLICA IDENTITY

The more I think about this, the more I think we're actually misusing 
REPLICA IDENTITY for something entirely different. The whole purpose of 
RI was to provide a row identifier for the subscriber.

But now we're using it to ensure we have all the necessary columns, 
which is entirely orthogonal to the original purpose. I predict this 
will have rather negative consequences.

People will either switch everything to REPLICA IDENTITY FULL, or create 
bogus unique indexes with extra columns. Which is really silly, because 
it wastes network bandwidth (transfers more data) or local resources 
(CPU and disk space to maintain extra indexes).

IMHO this needs more infrastructure to request extra columns to decode 
(e.g. for the filter expression), and then remove them before sending 
the data to the subscriber.


13) turning update into insert

I agree with Ajin Cherian [4] that looking at just old or new row for 
updates is not the right solution, because each option will "break" the 
replica in some case. So I think the goal "keeping the replica in sync" 
is the right perspective, and converting the update to insert/delete if 
needed seems appropriate.

This seems a somewhat similar to what pglogical does, because that may 
also convert updates (although only to inserts, IIRC) when handling 
replication conflicts. The difference is pglogical does all this on the 
subscriber, while this makes the decision on the publisher.

I wonder if this might have some negative consequences, or whether 
"moving" this to downstream would be useful for other purposes in the 
fuure (e.g. it might be reused for handling other conflicts).


14) pgoutput_row_filter_update

The function name seems a bit misleading, as it suggests might seem like 
it updates the row_filter, or something. Should indicate it's about 
deciding what to do with the update.


15) pgoutput_row_filter initializing filter

I'm not sure I understand why the filter initialization gets moved from 
get_rel_sync_entry. Presumably, most of what the replication does is 
replicating rows, so I see little point in not initializing this along 
with the rest of the rel_sync_entry.


regards


[1] 
https://www.postgresql.org/message-id/849ee491-bba3-c0ae-cc25-4fce1c03f105%40enterprisedb.com

[2] 
https://www.postgresql.org/message-id/7106a0fc-8017-c0fe-a407-9466c9407ff8%402ndquadrant.com

[3] 
https://www.postgresql.org/message-id/CAHut%2BPukNh_HsN1Au1p9YhG5KCOr3dH5jnwm%3DRmeX75BOtXTEg%40mail.gmail.com

[4] 
https://www.postgresql.org/message-id/CAFPTHDb7bpkuc4SxaL9B5vEvF2aEi0EOERdrG%2BxgVeAyMJsF%3DQ%40mail.gmail.com

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