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