Horton
Menu

Re: Pgoutput not capturing the generated columns

Shlok Kyal <shlok.kyal.oss@gmail.com>

View thread
From: Shlok Kyal <shlok.kyal.oss@gmail.com>
To: Peter Smith <smithpb2250@gmail.com>
Cc: Shubham Khanna <khannashubham1197@gmail.com>, Rajendra Kumar Dangwal <dangwalrajendra888@gmail.com>, pgsql-hackers@lists.postgresql.org, euler@eulerto.com
Date: 2024-07-16T08:57:44Z
Lists: pgsql-hackers

Attachments

On Tue, 9 Jul 2024 at 07:14, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Shlok, Here are my review comments for v16-0002
>
> ======
> src/test/subscription/t/004_sync.pl
>
> > > 5.
> > > Here, you are confirming we get an ERROR when replicating from a
> > > non-generated column to a generated column. But I think your patch
> > > also added exactly that same test scenario in the 011_generated (as
> > > the sub5 test). So, maybe this one here should be removed?
> >
> > For 0004_sync.pl, it is tested when 'include_generated_columns' is not
> > specified. Whereas for the test in 011_generated
> > 'include_generated_columns = true' is specified.
> > I thought we should have a test for both cases to test if the error
> > message format is the same for both cases. Thoughts?
>
> 3.
> Sorry, I missed that there was a parameter flag difference. Anyway,
> since the code-path to reach this error is the same regardless of the
> 'include_generated_columns' parameter value IMO having too many tests
> might be overkill. YMMV.
>
> Anyway, whether you decide to keep both test cases or not, I think all
> testing related to generated column replication belongs in the new
> 001_generated.pl TAP file -- not here in 04_sync.pl
I have removed the test

> ======
> src/test/subscription/t/011_generated.pl
>
> 4. Untested scenarios for "missing col"?
>
> I have seen (in 04_sync.pl) missing column test cases for:
> - publisher not-generated col ==> subscriber missing column
>
> Maybe I am mistaken, but I don't recall seeing any test cases for:
> - publisher generated-col ==> subscriber missing col
>
> Unless they are already done somewhere, I think this scenario should
> be in 011_generated.pl. Furthermore, maybe it needs to be tested for
> both include_generated_columns = true / false, because if the
> parameter is false it should be OK, but if the parameter is true it
> should give ERROR.
 Have added the tests in 011_generated.pl

I have also addressed the remaining comments. Please find the updated
v18 patches

v18-0001 - Rebased the patch on HEAD
v18-0002 - Addressed the comments
v18-0003 - Addressed the comments
v18-0004- Rebased the patch

Thanks and Regards,
Shlok Kyal

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: Generated column replication.

  2. Rename pubgencols_type to pubgencols in pg_publication.

  3. Doc: Fix column name in pg_publication catalog.

  4. Fix buildfarm failure introduced by commit e65dbc9927.

  5. Change publication's publish_generated_columns option type to enum.

  6. Fix \dRp+ output when describing publications with a lower server version.

  7. Replicate generated columns when 'publish_generated_columns' is set.

  8. Doc: Update the behavior of generated columns in Logical Replication.

  9. Replicate generated columns when specified in the column list.

  10. Doc: Generated columns are skipped for logical replication.