On 02.12.21 15:23, Alvaro Herrera wrote:
>> I took the latest posted patch, rebased on current sources, fixed the
>> conflicts, and pgindented. No further changes. Here's the result. All
>> tests are passing for me. Some review comments that were posted have
>> not been addressed yet; I'll look into that soon.
>
> In v7 I have reinstated the test file and fixed the silly problem that
> caused it to fail (probably a mistake of mine while rebasing).
I looked through this a bit. You had said that you are still going to
integrate past review comments, so I didn't look to deeply before you
get to that.
Attached are a few fixup patches that you could integrate.
There was no documentation, so I wrote a bit (patch 0001). It only
touches the CREATE PUBLICATION and ALTER PUBLICATION pages at the
moment. There was no mention in the Logical Replication chapter that
warranted updating. Perhaps we should revisit that chapter at the end
of the release cycle.
DDL tests should be done in src/test/regress/sql/publication.sql rather
than through TAP tests, to keep it simpler. I have added a few that I
came up with (patch 0002). Note the FIXME marker that it does not
recognize if the listed columns don't exist. I removed a now redundant
test from the TAP test file. The other error condition test in the TAP
test file ('publication relation test_part removed') I didn't
understand: test_part was added with columns (a, b), so why would
dropping column b remove the whole entry? Maybe I missed something, or
this could be explained better.
I was curious what happens when you have different publications with
different column lists, so I wrote a test for that (patch 0003). It
turns out it works, so there is nothing to do, but perhaps the test is
useful to keep.
The test file 021_column_filter.pl should be renamed to an unused number
(would be 027 currently). Also, it contains references to "TRUNCATE",
where it was presumably copied from.
On the implementation side, I think the added catalog column
pg_publication_rel.prattrs should be an int2 array, not a text array.
That would also fix the above problem. If you have to look up the
columns at DDL time, then you will notice when they don't exist.
Finally, I suggest not naming this feature "column filter". I think
this name arose because of the analogy with the "row filter" feature
also being developed. But a filter is normally a dynamic data-driven
action, which this is not. Golden Gate calls it in their documentation
"Selecting Columns", or we could just call it "column list".