Re: Column Filtering in Logical Replication

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