Horton
Menu

Re: Column Filtering in Logical Replication

Tomas Vondra <tomas.vondra@enterprisedb.com>

View thread
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>, Rahila Syed <rahilasyed90@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Peter Smith <smithpb2250@gmail.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2021-12-14T21:28:11Z
Lists: pgsql-hackers

Attachments

Hi,

I went through the v9 patch, and I have a couple comments / questions. 
Apologies if some of this was already discussed earlier, it's hard to 
cross-check in such a long thread. Most of the comments are in 0002 to 
make it easier to locate, and it also makes proposed code changes 
clearer I think.

1) check_publication_add_relation - the "else" branch is not really 
needed, because the "if (replidentfull)" always errors-out

2) publication_add_relation has a FIXME about handling cases with 
different column list

So what's the right behavior for ADD TABLE with different column list? 
I'd say we should allow that, and that it should be mostly the same 
thing as adding/removing columns to the list incrementally, i.e. we 
should replace the column lists. We could also prohibit such changes, 
but that seems like a really annoying limitation, forcing people to 
remove/add the relation.

I added some comments to the attmap translation block, and replaced <0 
check with AttrNumberIsForUserDefinedAttr.

But I wonder if we could get rid of the offset, considering we're 
dealing with just user-defined attributes. That'd make the code clearer, 
but it would break if we're comparing it to other bitmaps with offsets. 
But I don't think we do.

3) I doubt "att_map" is the right name, though. AFAICS it's just a list 
of columns for the relation, not a map, right? So maybe attr_list?

4) AlterPublication talks about "publication status" for a column, but 
do we actually track that? Or what does that mean?

5) PublicationDropTables does a check

     if (pubrel->columns)
         ereport(ERROR,
                 errcode(ERRCODE_SYNTAX_ERROR),

Shouldn't this be prevented by the grammar, really? Also, it should be 
in regression tests.

6) Another thing that should be in the test is partitioned table with 
attribute mapping and column list, to see how map and attr_map interact.

7) There's a couple places doing this

     if (att_map != NULL &&
         !bms_is_member(att->attnum - FirstLowInvalidHeapAttributeNumber,
                        att_map) &&
         !bms_is_member(att->attnum - FirstLowInvalidHeapAttributeNumber,
                        idattrs) &&
         !replidentfull)

which is really hard to understand (even if we get rid of the offset), 
so maybe let's move that to a function with sensible name. Also, some 
places don't check indattrs - seems a bit suspicious.


regards

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

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Doc: Explain about Column List feature.

  2. Doc: fix column list vs. replica identity rules.

  3. Prohibit combining publications with different column lists.

  4. Fix the check to limit sync workers.

  5. Wait for subscription to sync in t/031_column_list.sql

  6. Move prattrs to the pg_publication_rel section in docs

  7. Allow specifying column lists for logical replication

  8. Fix row filters with multiple publications

  9. Fix publish_as_relid with multiple publications

  10. Add some additional tests for row filters in logical replication.

  11. Add index on pg_publication_rel.prpubid

  12. Avoid using DefElemAction in AlterPublicationStmt

  13. Small cleanups related to PUBLICATION framework code

  14. Add PublicationTable and PublicationRelInfo structs

  15. Fix various concurrency issues in logical replication worker launching