Re: Pgoutput not capturing the generated columns

Shubham Khanna <khannashubham1197@gmail.com>

From: Shubham Khanna <khannashubham1197@gmail.com>
To: Shlok Kyal <shlok.kyal.oss@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>, Peter Smith <smithpb2250@gmail.com>, Rajendra Kumar Dangwal <dangwalrajendra888@gmail.com>, pgsql-hackers@lists.postgresql.org, euler@eulerto.com
Date: 2024-06-19T11:22:20Z
Lists: pgsql-hackers

Attachments

> Hi Shubham, thanks for providing a patch.
> I have some comments for v6-0001.
>
> 1. create_subscription.sgml
> There is repetition of the same line.
>
> +         <para>
> +          Specifies whether the generated columns present in the tables
> +          associated with the subscription should be replicated. If the
> +          subscriber-side column is also a generated column then this option
> +          has no effect; the replicated data will be ignored and the subscriber
> +          column will be filled as normal with the subscriber-side computed or
> +          default data.
> +          <literal>false</literal>.
> +         </para>
> +
> +         <para>
> +          This parameter can only be set true if
> <literal>copy_data</literal> is
> +          set to <literal>false</literal>. If the subscriber-side
> column is also a
> +          generated column then this option has no effect; the
> replicated data will
> +          be ignored and the subscriber column will be filled as
> normal with the
> +          subscriber-side computed or default data.
> +         </para>
>
> ==============================
> 2. subscriptioncmds.c
>
> 2a. The macro name should be in uppercase. We can use a short name
> like 'SUBOPT_INCLUDE_GEN_COL'. Thought?
> +#define SUBOPT_include_generated_columns 0x00010000
>
> 2b.Update macro name accordingly
> + if (IsSet(supported_opts, SUBOPT_include_generated_columns))
> + opts->include_generated_columns = false;
>
> 2c. Update macro name accordingly
> + else if (IsSet(supported_opts, SUBOPT_include_generated_columns) &&
> + strcmp(defel->defname, "include_generated_columns") == 0)
> + {
> + if (IsSet(opts->specified_opts, SUBOPT_include_generated_columns))
> + errorConflictingDefElem(defel, pstate);
> +
> + opts->specified_opts |= SUBOPT_include_generated_columns;
> + opts->include_generated_columns = defGetBoolean(defel);
> + }
>
> 2d. Update macro name accordingly
> +   SUBOPT_RUN_AS_OWNER | SUBOPT_FAILOVER | SUBOPT_ORIGIN |
> +   SUBOPT_include_generated_columns);
>
>
> ==============================
>
> 3. decoding_into_rel.out
>
> 3a. In comment, I think it should be "When 'include-generated-columns'
> = '1' the generated column 'b' values will be replicated"
> +-- When 'include-generated-columns' = '1' the generated column 'b'
> values will not be replicated
> +INSERT INTO gencoltable (a) VALUES (1), (2), (3);
> +SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL,
> NULL, 'include-xids', '0', 'skip-empty-xacts', '1',
> 'include-generated-columns', '1');
> +                            data
> +-------------------------------------------------------------
> + BEGIN
> + table public.gencoltable: INSERT: a[integer]:1 b[integer]:2
> + table public.gencoltable: INSERT: a[integer]:2 b[integer]:4
> + table public.gencoltable: INSERT: a[integer]:3 b[integer]:6
> + COMMIT
>
> 3b. In comment, I think it should be "When 'include-generated-columns'
> = '1' the generated column 'b' values will not be replicated"
> +-- When 'include-generated-columns' = '0' the generated column 'b'
> values will be replicated
> +SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL,
> NULL, 'include-xids', '0', 'skip-empty-xacts', '1',
> 'include-generated-columns', '0');
> +                      data
> +------------------------------------------------
> + BEGIN
> + table public.gencoltable: INSERT: a[integer]:4
> + table public.gencoltable: INSERT: a[integer]:5
> + table public.gencoltable: INSERT: a[integer]:6
> + COMMIT
> +(5 rows)
>
> =========================
>
> 4. Here names for both the tests are the same. I think we should use
> different names.
>
> +$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab2");
> +is( $result, qq(4|8
> +5|10), 'generated columns replicated to non-generated column on subscriber');
> +
> +$node_publisher->safe_psql('postgres', "INSERT INTO tab3 VALUES (4), (5)");
> +
> +$node_publisher->wait_for_catchup('sub3');
> +
> +$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab3");
> +is( $result, qq(4|24
> +5|25), 'generated columns replicated to non-generated column on subscriber');

All the comments are handled.

The attached Patch contains all the suggested changes.

Thanks and Regards,
Shubham Khanna.

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.