Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_decoding transform: skip toast columns with unchanged values #877

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

hanefi
Copy link
Contributor

@hanefi hanefi commented Sep 2, 2024

test_decoding plugin may not include the value of a TOAST column if it has not changed. In that case, instead of the value of the column the string "unchanged-toast-datum" is included in the output. In that case, we should remove the column from the range table entry list in the generated SQL queries instead of including it and setting that column to the value "unchanged-toast-datum".

There is one strange case that if a table has only toast columns, and an update query that does not update any value is executed, we end up filtering all the range table entries and skip executing any sql queries as it will be no-op.

Fixes: #350
Fixes: #710

test_decoding plugin may not include the value of a TOAST column if it
has not changed. In that case, instead of the value of the column the
string "unchanged-toast-datum" is included in the output. In that case,
we should remove the column from the range table entry list in the
generated SQL queries instead of including it and setting that column to
the value "unchanged-toast-datum".

There is one strange case that if a table has only toast columns, and an
update query that does not update any value is executed, we end up
filtering all the range table entries and skip executing any sql queries
as it will be no-op.
Copy link
Owner

@dimitri dimitri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it could be worth adding a SQL query to the tests to show that the values are indeed “uncompressed external” or “compressed external”. I can't remember how to do that, but I think it is possible to check at run-time?

@dimitri dimitri added the bug Something isn't working label Sep 2, 2024
@dimitri dimitri added this to the v0.18 milestone Sep 2, 2024
@hanefi
Copy link
Contributor Author

hanefi commented Sep 4, 2024

It looks like it could be worth adding a SQL query to the tests to show that the values are indeed “uncompressed external” or “compressed external”. I can't remember how to do that, but I think it is possible to check at run-time?

Checking for storage type:

select att.attname,
       case att.attstorage
          when 'p' then 'plain'
          when 'm' then 'main'
          when 'e' then 'external'
          when 'x' then 'extended'
       end as attstorage
from pg_attribute att
  join pg_class tbl on tbl.oid = att.attrelid
  join pg_namespace ns on tbl.relnamespace = ns.oid
where tbl.relname = 'xpto'
  and ns.nspname = 'public'
  and not att.attisdropped
order by 1;

+--------------+------------+
|   attname    | attstorage |
+--------------+------------+
| cmax         | plain      |
| cmin         | plain      |
| ctid         | plain      |
| id           | plain      |
| rand1        | plain      |
| rand2        | plain      |
| tableoid     | plain      |
| toasted_col1 | extended   |
| toasted_col2 | extended   |
| xmax         | plain      |
| xmin         | plain      |
+--------------+------------+
#define  TYPSTORAGE_PLAIN		'p' /* type not prepared for toasting */
#define  TYPSTORAGE_EXTERNAL	'e' /* toastable, don't try to compress */
#define  TYPSTORAGE_EXTENDED	'x' /* fully toastable */
#define  TYPSTORAGE_MAIN		'm' /* like 'x' but try to store inline */

Checking for column compression:

select id,
       pg_column_compression(toasted_col1) toasted_col1_compression,
       pg_column_compression(toasted_col2) toasted_col2_compression
from xpto
order by 1;

+----+--------------------------+--------------------------+
| id | toasted_col1_compression | toasted_col2_compression |
+----+--------------------------+--------------------------+
|  1 | (null)                   | (null)                   |
|  2 | (null)                   | pglz                     |
+----+--------------------------+--------------------------+

It turns out that the comments were partially wrong. We have extended storage instead of external. I guess the 11 year old PG tests are also misleading as I copied the table definitions from them. https://github.com/postgres/postgres/blob/82b07eba9e8b863cc05adb7e53a86ff02b51d888/contrib/test_decoding/sql/toast.sql#L17-L21

@dimitri dimitri merged commit ac1c193 into dimitri:main Sep 5, 2024
20 checks passed
@dimitri
Copy link
Owner

dimitri commented Sep 5, 2024

Merged as it is, please see if you want to add tests coverage for actual storage options of the columns in the unit tests. The goal I have in mind is to make sure that the actual Postgres behavior conforms with the unit tests expectations: limits could change in a new major version of Postgres in a way that we need to adjust the test data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with toast column in cdc apply for test_decoding Parsing error with test_decoding plugin
2 participants