public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 07/10] add wal_compression_method: zstd 9+ messages / 5 participants [nested] [flat]
* [PATCH 07/10] add wal_compression_method: zstd @ 2021-03-12 20:43 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 9+ messages in thread From: Justin Pryzby @ 2021-03-12 20:43 UTC (permalink / raw) --- configure | 163 ++++++++++++++++++ configure.ac | 26 +++ doc/src/sgml/config.sgml | 2 +- src/backend/access/transam/xlog.c | 3 + src/backend/access/transam/xloginsert.c | 18 +- src/backend/access/transam/xlogreader.c | 15 ++ src/backend/utils/misc/postgresql.conf.sample | 2 +- src/include/access/xlog_internal.h | 1 + src/include/pg_config.h.in | 3 + src/tools/msvc/Solution.pm | 1 + 10 files changed, 231 insertions(+), 3 deletions(-) diff --git a/configure b/configure index 8d76be00c1..81e23418b2 100755 --- a/configure +++ b/configure @@ -699,6 +699,9 @@ with_gnu_ld LD LDFLAGS_SL LDFLAGS_EX +ZSTD_LIBS +ZSTD_CFLAGS +with_zstd LZ4_LIBS LZ4_CFLAGS with_lz4 @@ -868,6 +871,7 @@ with_libxslt with_system_tzdata with_zlib with_lz4 +with_zstd with_gnu_ld with_ssl with_openssl @@ -897,6 +901,8 @@ XML2_CFLAGS XML2_LIBS LZ4_CFLAGS LZ4_LIBS +ZSTD_CFLAGS +ZSTD_LIBS LDFLAGS_EX LDFLAGS_SL PERL @@ -1576,6 +1582,7 @@ Optional Packages: use system time zone data in DIR --without-zlib do not use Zlib --without-lz4 build without LZ4 support + --with-zstd build with Zstd compression library --with-gnu-ld assume the C compiler uses GNU ld [default=no] --with-ssl=LIB use LIB for SSL/TLS support (openssl) --with-openssl obsolete spelling of --with-ssl=openssl @@ -1605,6 +1612,8 @@ Some influential environment variables: XML2_LIBS linker flags for XML2, overriding pkg-config LZ4_CFLAGS C compiler flags for LZ4, overriding pkg-config LZ4_LIBS linker flags for LZ4, overriding pkg-config + ZSTD_CFLAGS C compiler flags for ZSTD, overriding pkg-config + ZSTD_LIBS linker flags for ZSTD, overriding pkg-config LDFLAGS_EX extra linker flags for linking executables only LDFLAGS_SL extra linker flags for linking shared libraries only PERL Perl program @@ -8705,6 +8714,137 @@ fi CFLAGS="$LZ4_CFLAGS $CFLAGS" fi +# +# ZSTD +# +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether to build with zstd support" >&5 +$as_echo_n "checking whether to build with zstd support... " >&6; } + + + +# Check whether --with-zstd was given. +if test "${with_zstd+set}" = set; then : + withval=$with_zstd; + case $withval in + yes) + +$as_echo "#define USE_ZSTD 1" >>confdefs.h + + ;; + no) + : + ;; + *) + as_fn_error $? "no argument expected for --with-zstd option" "$LINENO" 5 + ;; + esac + +else + with_zstd=no + +fi + + +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $with_zstd" >&5 +$as_echo "$with_zstd" >&6; } + + +if test "$with_zstd" = yes; then + +pkg_failed=no +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for libzstd" >&5 +$as_echo_n "checking for libzstd... " >&6; } + +if test -n "$ZSTD_CFLAGS"; then + pkg_cv_ZSTD_CFLAGS="$ZSTD_CFLAGS" + elif test -n "$PKG_CONFIG"; then + if test -n "$PKG_CONFIG" && \ + { { $as_echo "$as_me:${as_lineno-$LINENO}: \$PKG_CONFIG --exists --print-errors \"libzstd\""; } >&5 + ($PKG_CONFIG --exists --print-errors "libzstd") 2>&5 + ac_status=$? + $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5 + test $ac_status = 0; }; then + pkg_cv_ZSTD_CFLAGS=`$PKG_CONFIG --cflags "libzstd" 2>/dev/null` + test "x$?" != "x0" && pkg_failed=yes +else + pkg_failed=yes +fi + else + pkg_failed=untried +fi +if test -n "$ZSTD_LIBS"; then + pkg_cv_ZSTD_LIBS="$ZSTD_LIBS" + elif test -n "$PKG_CONFIG"; then + if test -n "$PKG_CONFIG" && \ + { { $as_echo "$as_me:${as_lineno-$LINENO}: \$PKG_CONFIG --exists --print-errors \"libzstd\""; } >&5 + ($PKG_CONFIG --exists --print-errors "libzstd") 2>&5 + ac_status=$? + $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5 + test $ac_status = 0; }; then + pkg_cv_ZSTD_LIBS=`$PKG_CONFIG --libs "libzstd" 2>/dev/null` + test "x$?" != "x0" && pkg_failed=yes +else + pkg_failed=yes +fi + else + pkg_failed=untried +fi + + + +if test $pkg_failed = yes; then + { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5 +$as_echo "no" >&6; } + +if $PKG_CONFIG --atleast-pkgconfig-version 0.20; then + _pkg_short_errors_supported=yes +else + _pkg_short_errors_supported=no +fi + if test $_pkg_short_errors_supported = yes; then + ZSTD_PKG_ERRORS=`$PKG_CONFIG --short-errors --print-errors --cflags --libs "libzstd" 2>&1` + else + ZSTD_PKG_ERRORS=`$PKG_CONFIG --print-errors --cflags --libs "libzstd" 2>&1` + fi + # Put the nasty error message in config.log where it belongs + echo "$ZSTD_PKG_ERRORS" >&5 + + as_fn_error $? "Package requirements (libzstd) were not met: + +$ZSTD_PKG_ERRORS + +Consider adjusting the PKG_CONFIG_PATH environment variable if you +installed software in a non-standard prefix. + +Alternatively, you may set the environment variables ZSTD_CFLAGS +and ZSTD_LIBS to avoid the need to call pkg-config. +See the pkg-config man page for more details." "$LINENO" 5 +elif test $pkg_failed = untried; then + { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5 +$as_echo "no" >&6; } + { { $as_echo "$as_me:${as_lineno-$LINENO}: error: in \`$ac_pwd':" >&5 +$as_echo "$as_me: error: in \`$ac_pwd':" >&2;} +as_fn_error $? "The pkg-config script could not be found or is too old. Make sure it +is in your PATH or set the PKG_CONFIG environment variable to the full +path to pkg-config. + +Alternatively, you may set the environment variables ZSTD_CFLAGS +and ZSTD_LIBS to avoid the need to call pkg-config. +See the pkg-config man page for more details. + +To get pkg-config, see <http://pkg-config.freedesktop.org/;. +See \`config.log' for more details" "$LINENO" 5; } +else + ZSTD_CFLAGS=$pkg_cv_ZSTD_CFLAGS + ZSTD_LIBS=$pkg_cv_ZSTD_LIBS + { $as_echo "$as_me:${as_lineno-$LINENO}: result: yes" >&5 +$as_echo "yes" >&6; } + +fi + LIBS="$ZSTD_LIBS $LIBS" + CFLAGS="$ZSTD_CFLAGS $CFLAGS" +fi + # # Assignments # @@ -13559,6 +13699,29 @@ done CPPFLAGS=$ac_save_CPPFLAGS fi +if test "$with_zstd" = yes; then + ac_save_CPPFLAGS=$CPPFLAGS + CPPFLAGS="$ZSTD_CFLAGS $CPPFLAGS" + + # Verify we have zstd's header files + for ac_header in zstd.h +do : + ac_fn_c_check_header_mongrel "$LINENO" "zstd.h" "ac_cv_header_zstd_h" "$ac_includes_default" +if test "x$ac_cv_header_zstd_h" = xyes; then : + cat >>confdefs.h <<_ACEOF +#define HAVE_ZSTD_H 1 +_ACEOF + +else + as_fn_error $? "zstd.h header file is required for zstd" "$LINENO" 5 +fi + +done + + + CPPFLAGS=$ac_save_CPPFLAGS +fi + if test "$with_gssapi" = yes ; then for ac_header in gssapi/gssapi.h do : diff --git a/configure.ac b/configure.ac index bfcdc88be0..d6f6349067 100644 --- a/configure.ac +++ b/configure.ac @@ -1001,6 +1001,21 @@ if test "$with_lz4" = yes; then CFLAGS="$LZ4_CFLAGS $CFLAGS" fi +# +# ZSTD +# +AC_MSG_CHECKING([whether to build with zstd support]) +PGAC_ARG_BOOL(with, zstd, no, [build with Zstd compression library], + [AC_DEFINE([USE_ZSTD], 1, [Define to 1 to build with zstd support. (--with-zstd)])]) +AC_MSG_RESULT([$with_zstd]) +AC_SUBST(with_zstd) + +if test "$with_zstd" = yes; then + PKG_CHECK_MODULES(ZSTD, libzstd) + LIBS="$ZSTD_LIBS $LIBS" + CFLAGS="$ZSTD_CFLAGS $CFLAGS" +fi + # # Assignments # @@ -1436,6 +1451,17 @@ if test "$with_lz4" = yes; then CPPFLAGS=$ac_save_CPPFLAGS fi +if test "$with_zstd" = yes; then + ac_save_CPPFLAGS=$CPPFLAGS + CPPFLAGS="$ZSTD_CFLAGS $CPPFLAGS" + + # Verify we have zstd's header files + AC_CHECK_HEADERS(zstd.h, [], + [AC_MSG_ERROR([zstd.h header file is required for zstd])]) + + CPPFLAGS=$ac_save_CPPFLAGS +fi + if test "$with_gssapi" = yes ; then AC_CHECK_HEADERS(gssapi/gssapi.h, [], [AC_CHECK_HEADERS(gssapi.h, [], [AC_MSG_ERROR([gssapi.h header file is required for GSSAPI])])]) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 257775c83b..94dd6ef3e9 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -3083,7 +3083,7 @@ include_dir 'conf.d' This parameter selects the compression method used to compress WAL when <varname>wal_compression</varname> is enabled. The supported methods are pglz, zlib, and (if configured when - <productname>PostgreSQL</productname> was built) lz4. + <productname>PostgreSQL</productname> was built) lz4 and zstd. The default value is <literal>pglz</literal>. Only superusers can change this setting. </para> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 3657f74de9..307eee6626 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -189,6 +189,9 @@ const struct config_enum_entry wal_compression_options[] = { #endif #ifdef USE_LZ4 {"lz4", WAL_COMPRESSION_LZ4, false}, +#endif +#ifdef USE_ZSTD + {"zstd", WAL_COMPRESSION_ZSTD, false}, #endif {NULL, 0, false} }; diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c index 3c15286bd8..4591e476c6 100644 --- a/src/backend/access/transam/xloginsert.c +++ b/src/backend/access/transam/xloginsert.c @@ -48,10 +48,17 @@ #define LZ4_MAX_BLCKSZ 0 #endif +#ifdef USE_ZSTD +#include "zstd.h" +#define ZSTD_MAX_BLCKSZ ZSTD_COMPRESSBOUND(BLCKSZ) +#else +#define ZSTD_MAX_BLCKSZ 0 +#endif + /* Buffer size required to store a compressed version of backup block image */ #define PGLZ_MAX_BLCKSZ PGLZ_MAX_OUTPUT(BLCKSZ) -#define COMPRESS_BUFSIZE Max(Max(PGLZ_MAX_BLCKSZ, ZLIB_MAX_BLCKSZ), LZ4_MAX_BLCKSZ) +#define COMPRESS_BUFSIZE Max(Max(Max(PGLZ_MAX_BLCKSZ, ZLIB_MAX_BLCKSZ), LZ4_MAX_BLCKSZ), ZSTD_MAX_BLCKSZ) /* * For each block reference registered with XLogRegisterBuffer, we fill in @@ -906,6 +913,15 @@ XLogCompressBackupBlock(char *page, uint16 hole_offset, uint16 hole_length, break; #endif +#ifdef USE_ZSTD + case WAL_COMPRESSION_ZSTD: + len = ZSTD_compress(dest, COMPRESS_BUFSIZE, source, orig_len, + ZSTD_CLEVEL_DEFAULT); + if (ZSTD_isError(len)) + len = -1; + break; +#endif + default: /* * It should be impossible to get here for unsupported algorithms, diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c index 97165f1bb1..0f9d522087 100644 --- a/src/backend/access/transam/xlogreader.c +++ b/src/backend/access/transam/xlogreader.c @@ -41,6 +41,10 @@ #include "lz4.h" #endif +#ifdef USE_ZSTD +#include "zstd.h" +#endif + static void report_invalid_record(XLogReaderState *state, const char *fmt,...) pg_attribute_printf(2, 3); static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength); @@ -1549,6 +1553,7 @@ struct walcompression walmethods[] = { {"pglz", WAL_COMPRESSION_PGLZ}, {"zlib", WAL_COMPRESSION_ZLIB}, {"lz4", WAL_COMPRESSION_LZ4}, + {"zstd", WAL_COMPRESSION_ZSTD}, }; /* @@ -1621,6 +1626,16 @@ RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page) break; #endif +#ifdef USE_ZSTD + case WAL_COMPRESSION_ZSTD: + decomp_result = ZSTD_decompress(tmp.data, BLCKSZ-bkpb->hole_length, + ptr, bkpb->bimg_len); + // XXX: ZSTD_getErrorName + if (ZSTD_isError(decomp_result)) + decomp_result = -1; + break; +#endif + default: report_invalid_record(record, "image at %X/%X is compressed with unsupported codec, block %d (%d/%s)", (uint32) (record->ReadRecPtr >> 32), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index 76f494cb9b..d372e2a817 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -213,7 +213,7 @@ # open_sync #full_page_writes = on # recover from partial page writes #wal_compression = off # enable compression of full-page writes -#wal_compression_method = pglz # pglz, zlib, lz4 +#wal_compression_method = pglz # pglz, zlib, lz4, zstd #wal_log_hints = off # also do full page writes of non-critical updates # (change requires restart) #wal_init_zero = on # zero-fill new WAL files diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h index b908fa48de..fa8146645d 100644 --- a/src/include/access/xlog_internal.h +++ b/src/include/access/xlog_internal.h @@ -337,6 +337,7 @@ typedef enum WalCompression WAL_COMPRESSION_PGLZ, WAL_COMPRESSION_ZLIB, WAL_COMPRESSION_LZ4, + WAL_COMPRESSION_ZSTD, } WalCompression; extern const char *wal_compression_name(WalCompression compression); diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index 0a6422da4f..ad26393352 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -902,6 +902,9 @@ /* Define to 1 to build with LZ4 support (--with-lz4) */ #undef USE_LZ4 +/* Define to 1 if you have the `zstd' library (-lzstd). */ +#undef USE_ZSTD + /* Define to select named POSIX semaphores. */ #undef USE_NAMED_POSIX_SEMAPHORES diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm index 14605371bb..fff0212087 100644 --- a/src/tools/msvc/Solution.pm +++ b/src/tools/msvc/Solution.pm @@ -486,6 +486,7 @@ sub GenerateFiles USE_LIBXML => undef, USE_LIBXSLT => undef, USE_LZ4 => undef, + USE_ZSTD => $self->{options}->{zstd} ? 1 : undef, USE_LDAP => $self->{options}->{ldap} ? 1 : undef, USE_LLVM => undef, USE_NAMED_POSIX_SEMAPHORES => undef, -- 2.17.0 --jozmn01XJZjDjM3N Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0008-Default-to-zstd.patch" ^ permalink raw reply [nested|flat] 9+ messages in thread
* RE: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL @ 2023-03-13 10:53 [email protected] <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: [email protected] @ 2023-03-13 10:53 UTC (permalink / raw) To: Önder Kalacı <[email protected]>; PostgreSQL Hackers <[email protected]>; +Cc: Amit Kapila <[email protected]> On Sun, Mar 12, 2023 4:00 AM Önder Kalacı <[email protected]> wrote: > > Attaching a patch that could possibly solve the problem. > Thanks for your patch. I tried it and it worked well. Here are some minor comments. 1. @@ -243,6 +243,17 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2, Form_pg_attribute att; TypeCacheEntry *typentry; + + Form_pg_attribute attr = TupleDescAttr(slot1->tts_tupleDescriptor, attrnum); + I think we can use "att" instead of a new variable. They have the same value. 2. +# The bug was that when when the REPLICA IDENTITY FULL is used with dropped There is an extra "when". Regards, Shi Yu ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL @ 2023-03-13 12:56 Önder Kalacı <[email protected]> parent: [email protected] <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: Önder Kalacı @ 2023-03-13 12:56 UTC (permalink / raw) To: [email protected] <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Amit Kapila <[email protected]> Hi Shi Yu, > 1. > @@ -243,6 +243,17 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot > *slot2, > Form_pg_attribute att; > TypeCacheEntry *typentry; > > + > + Form_pg_attribute attr = > TupleDescAttr(slot1->tts_tupleDescriptor, attrnum); > + > > I think we can use "att" instead of a new variable. They have the same > value. > ah, of course :) > > 2. > +# The bug was that when when the REPLICA IDENTITY FULL is used with > dropped > > There is an extra "when". > > Fixed, thanks Attaching v2 Attachments: [application/x-patch] v2-0001-Skip-dropped-and-generated-columns-when-REPLICA-I.patch (4.2K, ../../CACawEhUCg-tRGzRqRq+qHaR+kPdZ=N1fP=A4_cCYd0LuNLUgnA@mail.gmail.com/3-v2-0001-Skip-dropped-and-generated-columns-when-REPLICA-I.patch) download | inline diff: From caae511206ee904db54d0a9300ecef04c86aadb6 Mon Sep 17 00:00:00 2001 From: Onder Kalaci <[email protected]> Date: Sat, 11 Mar 2023 12:09:31 +0300 Subject: [PATCH v2] Skip dropped and generated columns when REPLICA IDENTITY FULL Dropped columns and generated columns are filled with NULL values on slot_store_data() but not on table_scan_getnextslot(). With this commit, we skip such columns while checking tuple equality. --- src/backend/executor/execReplication.c | 10 ++++ src/test/subscription/t/100_bugs.pl | 67 ++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index 4f5083a598..9d447b3c00 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -243,6 +243,16 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2, Form_pg_attribute att; TypeCacheEntry *typentry; + att = TupleDescAttr(slot1->tts_tupleDescriptor, attrnum); + + /* + * Dropped columns and generated columns are filled with NULL values on + * slot_store_data() but not on table_scan_getnextslot, so ignore + * for the comparison. + */ + if (att->attisdropped || att->attgenerated) + continue; + /* * If one value is NULL and other is not, then they are certainly not * equal diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl index 036576acab..5f902d26b6 100644 --- a/src/test/subscription/t/100_bugs.pl +++ b/src/test/subscription/t/100_bugs.pl @@ -373,4 +373,71 @@ is( $result, qq(1 $node_publisher->stop('fast'); $node_subscriber->stop('fast'); +# https://www.postgresql.org/message-id/CACawEhUu6S8E4Oo7%2Bs5iaq%3DyLRZJb6uOZeEQSGJj-7NVkDzSaw%40mail.gmail.com + +# The bug was that when the REPLICA IDENTITY FULL is used with dropped +# or generated columns, the equality checks for the tuples was trying to +# compare NULL Datum values (coming from slot_store_data()) with the +# non-visible actua Datum values (coming from table_scan_getnextslot()). +my $node_publisher_d_cols = PostgreSQL::Test::Cluster->new('node_publisher_d_cols'); +$node_publisher_d_cols->init(allows_streaming => 'logical'); +$node_publisher_d_cols->start; + +my $node_subscriber_d_cols = PostgreSQL::Test::Cluster->new('node_subscriber_d_cols'); +$node_subscriber_d_cols->init(allows_streaming => 'logical'); +$node_subscriber_d_cols->start; + +$node_publisher_d_cols->safe_psql( + 'postgres', qq( + CREATE TABLE dropped_cols (a int, b_drop int, c int); + ALTER TABLE dropped_cols REPLICA IDENTITY FULL; + + CREATE TABLE generated_cols (a int, b_gen int GENERATED ALWAYS AS (5 * a) STORED, c int); + ALTER TABLE generated_cols REPLICA IDENTITY FULL; + + CREATE PUBLICATION pub_dropped_cols FOR TABLE dropped_cols, generated_cols; + + -- some initial data + INSERT INTO dropped_cols VALUES (1, 1, 1); + INSERT INTO generated_cols (a,c) VALUES (1, 1); +)); + +$node_subscriber_d_cols->safe_psql( + 'postgres', qq( + CREATE TABLE dropped_cols (a int, b_drop int, c int); + CREATE TABLE generated_cols (a int, b_gen int GENERATED ALWAYS AS (5 * a) STORED, c int); + +)); + +my $publisher_connstr_d_cols = $node_publisher_d_cols->connstr . ' dbname=postgres'; +$node_subscriber_d_cols->safe_psql('postgres', + "CREATE SUBSCRIPTION sub_dropped_cols CONNECTION '$publisher_connstr_d_cols' PUBLICATION pub_dropped_cols" +); +$node_subscriber_d_cols->wait_for_subscription_sync; + +$node_publisher_d_cols->safe_psql( + 'postgres', qq( + ALTER TABLE dropped_cols DROP COLUMN b_drop; +)); +$node_subscriber_d_cols->safe_psql( + 'postgres', qq( + ALTER TABLE dropped_cols DROP COLUMN b_drop; +)); + +$node_publisher_d_cols->safe_psql( + 'postgres', qq( + UPDATE dropped_cols SET a = 100; + UPDATE generated_cols SET a = 100; +)); +$node_publisher_d_cols->wait_for_catchup('sub_dropped_cols'); + +is($node_subscriber_d_cols->safe_psql('postgres', "SELECT count(*) FROM dropped_cols WHERE a = 100"), + qq(1), 'replication with RI FULL and dropped columns'); + +is($node_subscriber_d_cols->safe_psql('postgres', "SELECT count(*) FROM generated_cols WHERE a = 100"), + qq(1), 'replication with RI FULL and generated columns'); + +$node_publisher_d_cols->stop('fast'); +$node_subscriber_d_cols->stop('fast'); + done_testing(); -- 2.34.1 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL @ 2023-03-16 09:23 Amit Kapila <[email protected]> parent: Önder Kalacı <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: Amit Kapila @ 2023-03-16 09:23 UTC (permalink / raw) To: Önder Kalacı <[email protected]>; +Cc: [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, Mar 13, 2023 at 6:26 PM Önder Kalacı <[email protected]> wrote: > > Attaching v2 > Can we change the comment to: "Ignore dropped and generated columns as the publisher doesn't send those."? After your change, att = TupleDescAttr(slot1->tts_tupleDescriptor, attrnum); is done twice in the same function. In test cases, let's change the comment to: "The bug was that when the REPLICA IDENTITY FULL is used with dropped or generated columns, we fail to apply updates and deletes.". Also, I think we don't need to provide the email link as anyway commit message will have a link to the discussion. Did you check this in the back branches? -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 9+ messages in thread
* RE: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL @ 2023-03-16 10:38 [email protected] <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: [email protected] @ 2023-03-16 10:38 UTC (permalink / raw) To: Amit Kapila <[email protected]>; Önder Kalacı <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Thu, Mar 16, 2023 5:23 PM Amit Kapila <[email protected]> wrote: > > On Mon, Mar 13, 2023 at 6:26 PM Önder Kalacı <[email protected]> > wrote: > > > > Attaching v2 > > > > Can we change the comment to: "Ignore dropped and generated columns as > the publisher doesn't send those."? After your change, att = > TupleDescAttr(slot1->tts_tupleDescriptor, attrnum); is done twice in > the same function. > > In test cases, let's change the comment to: "The bug was that when the > REPLICA IDENTITY FULL is used with dropped or generated columns, we > fail to apply updates and deletes.". Also, I think we don't need to > provide the email link as anyway commit message will have a link to > the discussion. > > Did you check this in the back branches? > I tried to reproduce this bug in backbranch. Generated column is introduced in PG12, and I reproduced generated column problem in PG12~PG15. For dropped column problem, I reproduced it in PG10~PG15. (Logical replication was introduced in PG10) So I think we should backpatch the fix for generated column to PG12, and backpatch the fix for dropped column to PG10. Regards, Shi Yu ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL @ 2023-03-16 16:03 Önder Kalacı <[email protected]> parent: [email protected] <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: Önder Kalacı @ 2023-03-16 16:03 UTC (permalink / raw) To: [email protected] <[email protected]>; +Cc: Amit Kapila <[email protected]>; PostgreSQL Hackers <[email protected]> Hi Amit, Shi Yu, > Generated column is introduced in PG12, and I reproduced generated column problem in PG12~PG15. > For dropped column problem, I reproduced it in PG10~PG15. (Logical replication was introduced in PG10) So, I'm planning to split the changes into two commits. The first one fixes for dropped columns, and the second one adds generated columns check/test. Is that the right approach for such a case? > and backpatch the fix for dropped column to PG10. Still, even the first commit fails to apply cleanly to PG12 (and below). What is the procedure here? Should I be creating multiple patches per version? Or does the committer prefer to handle the conflicts? Depending on your reply, I can work on the followup. I'm still attaching the dropped column patch for reference. Thanks, Onder [email protected] <[email protected]>, 16 Mar 2023 Per, 13:38 tarihinde şunu yazdı: > On Thu, Mar 16, 2023 5:23 PM Amit Kapila <[email protected]> wrote: > > > > On Mon, Mar 13, 2023 at 6:26 PM Önder Kalacı <[email protected]> > > wrote: > > > > > > Attaching v2 > > > > > > > Can we change the comment to: "Ignore dropped and generated columns as > > the publisher doesn't send those."? After your change, att = > > TupleDescAttr(slot1->tts_tupleDescriptor, attrnum); is done twice in > > the same function. > > > > In test cases, let's change the comment to: "The bug was that when the > > REPLICA IDENTITY FULL is used with dropped or generated columns, we > > fail to apply updates and deletes.". Also, I think we don't need to > > provide the email link as anyway commit message will have a link to > > the discussion. > > > > Did you check this in the back branches? > > > > I tried to reproduce this bug in backbranch. > > Generated column is introduced in PG12, and I reproduced generated column > problem > in PG12~PG15. > > For dropped column problem, I reproduced it in PG10~PG15. (Logical > replication > was introduced in PG10) > > So I think we should backpatch the fix for generated column to PG12, and > backpatch the fix for dropped column to PG10. > > Regards, > Shi Yu > Attachments: [application/x-patch] v1-0001-Ignore-dropped-columns-when-REPLICA-IDENTITY-FULL.patch (3.6K, ../../CACawEhVCoCj5n1LUWBd12gd1S9ONkB3jf7fw5kK5njA==1X3rg@mail.gmail.com/3-v1-0001-Ignore-dropped-columns-when-REPLICA-IDENTITY-FULL.patch) download | inline diff: From d25b2feac3b26646034bf3ace6ab93056c3c29af Mon Sep 17 00:00:00 2001 From: Onder Kalaci <[email protected]> Date: Thu, 16 Mar 2023 18:06:54 +0300 Subject: [PATCH v1 1/2] Ignore dropped columns when REPLICA IDENTITY FULL Dropped columns are filled with NULL values on slot_store_data() but not on table_scan_getnextslot(). With this commit, we skip such columns while checking tuple equality. --- src/backend/executor/execReplication.c | 9 ++++- src/test/subscription/t/100_bugs.pl | 51 ++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index fa8628e3e1..7c8b0d2667 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -289,6 +289,13 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2, Form_pg_attribute att; TypeCacheEntry *typentry; + /* + * Ignore dropped columns as the publisher doesn't send those + */ + att = TupleDescAttr(slot1->tts_tupleDescriptor, attrnum); + if (att->attisdropped) + continue; + /* * If one value is NULL and other is not, then they are certainly not * equal @@ -302,8 +309,6 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2, if (slot1->tts_isnull[attrnum] || slot2->tts_isnull[attrnum]) continue; - att = TupleDescAttr(slot1->tts_tupleDescriptor, attrnum); - typentry = eq[attrnum]; if (typentry == NULL) { diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl index 036576acab..255746094c 100644 --- a/src/test/subscription/t/100_bugs.pl +++ b/src/test/subscription/t/100_bugs.pl @@ -373,4 +373,55 @@ is( $result, qq(1 $node_publisher->stop('fast'); $node_subscriber->stop('fast'); +# The bug was that when the REPLICA IDENTITY FULL is used with dropped columns, +# we fail to apply updates and deletes +my $node_publisher_d_cols = PostgreSQL::Test::Cluster->new('node_publisher_d_cols'); +$node_publisher_d_cols->init(allows_streaming => 'logical'); +$node_publisher_d_cols->start; + +my $node_subscriber_d_cols = PostgreSQL::Test::Cluster->new('node_subscriber_d_cols'); +$node_subscriber_d_cols->init(allows_streaming => 'logical'); +$node_subscriber_d_cols->start; + +$node_publisher_d_cols->safe_psql( + 'postgres', qq( + CREATE TABLE dropped_cols (a int, b_drop int, c int); + ALTER TABLE dropped_cols REPLICA IDENTITY FULL; + CREATE PUBLICATION pub_dropped_cols FOR TABLE dropped_cols; + -- some initial data + INSERT INTO dropped_cols VALUES (1, 1, 1); +)); + +$node_subscriber_d_cols->safe_psql( + 'postgres', qq( + CREATE TABLE dropped_cols (a int, b_drop int, c int); +)); + +my $publisher_connstr_d_cols = $node_publisher_d_cols->connstr . ' dbname=postgres'; +$node_subscriber_d_cols->safe_psql('postgres', + "CREATE SUBSCRIPTION sub_dropped_cols CONNECTION '$publisher_connstr_d_cols' PUBLICATION pub_dropped_cols" +); +$node_subscriber_d_cols->wait_for_subscription_sync; + +$node_publisher_d_cols->safe_psql( + 'postgres', qq( + ALTER TABLE dropped_cols DROP COLUMN b_drop; +)); +$node_subscriber_d_cols->safe_psql( + 'postgres', qq( + ALTER TABLE dropped_cols DROP COLUMN b_drop; +)); + +$node_publisher_d_cols->safe_psql( + 'postgres', qq( + UPDATE dropped_cols SET a = 100; +)); +$node_publisher_d_cols->wait_for_catchup('sub_dropped_cols'); + +is($node_subscriber_d_cols->safe_psql('postgres', "SELECT count(*) FROM dropped_cols WHERE a = 100"), + qq(1), 'replication with RI FULL and dropped columns'); + +$node_publisher_d_cols->stop('fast'); +$node_subscriber_d_cols->stop('fast'); + done_testing(); -- 2.34.1 [application/x-patch] v1-0002-Ignore-generated-columns-when-REPLICA-IDENTITY-FU.patch (2.9K, ../../CACawEhVCoCj5n1LUWBd12gd1S9ONkB3jf7fw5kK5njA==1X3rg@mail.gmail.com/4-v1-0002-Ignore-generated-columns-when-REPLICA-IDENTITY-FU.patch) download | inline diff: From b84f38deac1b02f057bfc05ff868fbf4a621b18b Mon Sep 17 00:00:00 2001 From: Onder Kalaci <[email protected]> Date: Thu, 16 Mar 2023 18:18:37 +0300 Subject: [PATCH v1 2/2] Ignore generated columns when REPLICA IDENTITY FULL Generated columns are filled with NULL values on slot_store_data() but not on table_scan_getnextslot(). With this commit, we skip such columns while checking tuple equality. --- src/backend/executor/execReplication.c | 5 +++-- src/test/subscription/t/100_bugs.pl | 10 +++++++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index 7c8b0d2667..482806961a 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -290,10 +290,11 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2, TypeCacheEntry *typentry; /* - * Ignore dropped columns as the publisher doesn't send those + * Ignore dropped and generated columns as the publisher doesn't + * send those */ att = TupleDescAttr(slot1->tts_tupleDescriptor, attrnum); - if (att->attisdropped) + if (att->attisdropped || att->attgenerated) continue; /* diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl index 255746094c..96126428f3 100644 --- a/src/test/subscription/t/100_bugs.pl +++ b/src/test/subscription/t/100_bugs.pl @@ -387,14 +387,18 @@ $node_publisher_d_cols->safe_psql( 'postgres', qq( CREATE TABLE dropped_cols (a int, b_drop int, c int); ALTER TABLE dropped_cols REPLICA IDENTITY FULL; - CREATE PUBLICATION pub_dropped_cols FOR TABLE dropped_cols; + CREATE TABLE generated_cols (a int, b_gen int GENERATED ALWAYS AS (5 * a) STORED, c int); + ALTER TABLE generated_cols REPLICA IDENTITY FULL; + CREATE PUBLICATION pub_dropped_cols FOR TABLE dropped_cols, generated_cols; -- some initial data INSERT INTO dropped_cols VALUES (1, 1, 1); + INSERT INTO generated_cols (a,c) VALUES (1, 1); )); $node_subscriber_d_cols->safe_psql( 'postgres', qq( CREATE TABLE dropped_cols (a int, b_drop int, c int); + CREATE TABLE generated_cols (a int, b_gen int GENERATED ALWAYS AS (5 * a) STORED, c int); )); my $publisher_connstr_d_cols = $node_publisher_d_cols->connstr . ' dbname=postgres'; @@ -415,12 +419,16 @@ $node_subscriber_d_cols->safe_psql( $node_publisher_d_cols->safe_psql( 'postgres', qq( UPDATE dropped_cols SET a = 100; + UPDATE generated_cols SET a = 100; )); $node_publisher_d_cols->wait_for_catchup('sub_dropped_cols'); is($node_subscriber_d_cols->safe_psql('postgres', "SELECT count(*) FROM dropped_cols WHERE a = 100"), qq(1), 'replication with RI FULL and dropped columns'); +is($node_subscriber_d_cols->safe_psql('postgres', "SELECT count(*) FROM generated_cols WHERE a = 100"), + qq(1), 'replication with RI FULL and generated columns'); + $node_publisher_d_cols->stop('fast'); $node_subscriber_d_cols->stop('fast'); -- 2.34.1 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL @ 2023-03-17 00:08 Amit Kapila <[email protected]> parent: Önder Kalacı <[email protected]> 0 siblings, 2 replies; 9+ messages in thread From: Amit Kapila @ 2023-03-17 00:08 UTC (permalink / raw) To: Önder Kalacı <[email protected]>; +Cc: [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, Mar 16, 2023 at 9:33 PM Önder Kalacı <[email protected]> wrote: > > Hi Amit, Shi Yu, > > > Generated column is introduced in PG12, and I reproduced generated column problem > in PG12~PG15. > > For dropped column problem, I reproduced it in PG10~PG15. (Logical replication > was introduced in PG10) > > So, I'm planning to split the changes into two commits. The first one fixes > for dropped columns, and the second one adds generated columns check/test. > > Is that the right approach for such a case? > Works for me. > > and backpatch the fix for dropped column to PG10. > > Still, even the first commit fails to apply cleanly to PG12 (and below). > > What is the procedure here? Should I be creating multiple patches per version? > You can first submit the fix for dropped columns with patches till v10. Once that is committed, then you can send the patches for generated columns. > Or does the committer prefer to handle the conflicts? Depending on your reply, > I can work on the followup. > Thanks for working on it. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL @ 2023-03-17 00:11 Tom Lane <[email protected]> parent: Amit Kapila <[email protected]> 1 sibling, 0 replies; 9+ messages in thread From: Tom Lane @ 2023-03-17 00:11 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Önder Kalacı <[email protected]>; [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]> Amit Kapila <[email protected]> writes: > On Thu, Mar 16, 2023 at 9:33 PM Önder Kalacı <[email protected]> wrote: >>> and backpatch the fix for dropped column to PG10. > You can first submit the fix for dropped columns with patches till > v10. Once that is committed, then you can send the patches for > generated columns. Don't worry about v10 --- that's out of support and shouldn't get patched for this. regards, tom lane ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL @ 2023-03-17 07:38 Önder Kalacı <[email protected]> parent: Amit Kapila <[email protected]> 1 sibling, 0 replies; 9+ messages in thread From: Önder Kalacı @ 2023-03-17 07:38 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]> Hi Amit, all > You can first submit the fix for dropped columns with patches till > v10. Once that is committed, then you can send the patches for > generated columns. > > Alright, attaching 2 patches for dropped columns, the names of the files shows which versions the patch can be applied to: v2-0001-Ignore-dropped-columns-HEAD-REL_15-REL_14-REL_13.patch v2-0001-Ignore-dropped-columns-REL_12-REL_11.patch And, then on top of that, you can apply the patch for generated columns on all applicable versions (HEAD, 15, 14, 13 and 12). It applies cleanly. The name of the file is: v2-0001-Ignore-generated-columns.patch But unfortunately I couldn't test the patch with PG 12 and below. I'm getting some unrelated compile errors and Postgrees CI is not available on these versions . I'll try to fix that, but I thought it would still be good to share the patches as you might already have the environment to run the tests. Don't worry about v10 --- that's out of support and shouldn't > get patched for this. Given this information, I skipped the v10 patch. Thanks, Onder KALACI Attachments: [application/octet-stream] v2-0001-Ignore-dropped-columns-REL_12-REL_11.patch (3.4K, ../../CACawEhWkFwQ5Zuu9NOQiJtjnQY+At8JuhKQ6cJ7wEvWv+Vb=+g@mail.gmail.com/3-v2-0001-Ignore-dropped-columns-REL_12-REL_11.patch) download | inline diff: From dd3682982ba7d1af866f72cbf4bad7b876fa68d9 Mon Sep 17 00:00:00 2001 From: Onder Kalaci <[email protected]> Date: Fri, 17 Mar 2023 09:39:15 +0300 Subject: [PATCH v1] Ignore dropped columns when REPLICA IDENTITY FULL Dropped columns are filled with NULL values on slot_store_data() but not on table_scan_getnextslot(). With this commit, we skip such columns while checking tuple equality. --- src/backend/executor/execReplication.c | 7 ++++ src/test/subscription/t/100_bugs.pl | 51 ++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index 6289322127..b3ba1aa6c8 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -242,6 +242,13 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2) Form_pg_attribute att; TypeCacheEntry *typentry; + /* + * Ignore dropped columns as the publisher doesn't send those + */ + att = TupleDescAttr(slot1->tts_tupleDescriptor, attrnum); + if (att->attisdropped) + continue; + /* * If one value is NULL and other is not, then they are certainly not * equal diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl index cbf46a3d33..3e7071de38 100644 --- a/src/test/subscription/t/100_bugs.pl +++ b/src/test/subscription/t/100_bugs.pl @@ -69,6 +69,57 @@ pass('index predicates do not cause crash'); $node_publisher->stop('fast'); $node_subscriber->stop('fast'); +# The bug was that when the REPLICA IDENTITY FULL is used with dropped columns, +# we fail to apply updates and deletes +my $node_publisher_d_cols = PostgreSQL::Test::Cluster->new('node_publisher_d_cols'); +$node_publisher_d_cols->init(allows_streaming => 'logical'); +$node_publisher_d_cols->start; + +my $node_subscriber_d_cols = PostgreSQL::Test::Cluster->new('node_subscriber_d_cols'); +$node_subscriber_d_cols->init(allows_streaming => 'logical'); +$node_subscriber_d_cols->start; + +$node_publisher_d_cols->safe_psql( + 'postgres', qq( + CREATE TABLE dropped_cols (a int, b_drop int, c int); + ALTER TABLE dropped_cols REPLICA IDENTITY FULL; + CREATE PUBLICATION pub_dropped_cols FOR TABLE dropped_cols; + -- some initial data + INSERT INTO dropped_cols VALUES (1, 1, 1); +)); + +$node_subscriber_d_cols->safe_psql( + 'postgres', qq( + CREATE TABLE dropped_cols (a int, b_drop int, c int); +)); + +my $publisher_connstr_d_cols = $node_publisher_d_cols->connstr . ' dbname=postgres'; +$node_subscriber_d_cols->safe_psql('postgres', + "CREATE SUBSCRIPTION sub_dropped_cols CONNECTION '$publisher_connstr_d_cols' PUBLICATION pub_dropped_cols" +); +$node_subscriber_d_cols->wait_for_subscription_sync; + +$node_publisher_d_cols->safe_psql( + 'postgres', qq( + ALTER TABLE dropped_cols DROP COLUMN b_drop; +)); +$node_subscriber_d_cols->safe_psql( + 'postgres', qq( + ALTER TABLE dropped_cols DROP COLUMN b_drop; +)); + +$node_publisher_d_cols->safe_psql( + 'postgres', qq( + UPDATE dropped_cols SET a = 100; +)); +$node_publisher_d_cols->wait_for_catchup('sub_dropped_cols'); + +is($node_subscriber_d_cols->safe_psql('postgres', "SELECT count(*) FROM dropped_cols WHERE a = 100"), + qq(1), 'replication with RI FULL and dropped columns'); + +$node_publisher_d_cols->stop('fast'); +$node_subscriber_d_cols->stop('fast'); + # Handling of temporary and unlogged tables with FOR ALL TABLES publications -- 2.34.1 [application/octet-stream] v2-0001-Ignore-dropped-columns-HEAD-REL_15-REL_14-REL_13.patch (3.6K, ../../CACawEhWkFwQ5Zuu9NOQiJtjnQY+At8JuhKQ6cJ7wEvWv+Vb=+g@mail.gmail.com/4-v2-0001-Ignore-dropped-columns-HEAD-REL_15-REL_14-REL_13.patch) download | inline diff: From d25b2feac3b26646034bf3ace6ab93056c3c29af Mon Sep 17 00:00:00 2001 From: Onder Kalaci <[email protected]> Date: Thu, 16 Mar 2023 18:06:54 +0300 Subject: [PATCH v1 1/2] Ignore dropped columns when REPLICA IDENTITY FULL Dropped columns are filled with NULL values on slot_store_data() but not on table_scan_getnextslot(). With this commit, we skip such columns while checking tuple equality. --- src/backend/executor/execReplication.c | 9 ++++- src/test/subscription/t/100_bugs.pl | 51 ++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index fa8628e3e1..7c8b0d2667 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -289,6 +289,13 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2, Form_pg_attribute att; TypeCacheEntry *typentry; + /* + * Ignore dropped columns as the publisher doesn't send those + */ + att = TupleDescAttr(slot1->tts_tupleDescriptor, attrnum); + if (att->attisdropped) + continue; + /* * If one value is NULL and other is not, then they are certainly not * equal @@ -302,8 +309,6 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2, if (slot1->tts_isnull[attrnum] || slot2->tts_isnull[attrnum]) continue; - att = TupleDescAttr(slot1->tts_tupleDescriptor, attrnum); - typentry = eq[attrnum]; if (typentry == NULL) { diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl index 036576acab..255746094c 100644 --- a/src/test/subscription/t/100_bugs.pl +++ b/src/test/subscription/t/100_bugs.pl @@ -373,4 +373,55 @@ is( $result, qq(1 $node_publisher->stop('fast'); $node_subscriber->stop('fast'); +# The bug was that when the REPLICA IDENTITY FULL is used with dropped columns, +# we fail to apply updates and deletes +my $node_publisher_d_cols = PostgreSQL::Test::Cluster->new('node_publisher_d_cols'); +$node_publisher_d_cols->init(allows_streaming => 'logical'); +$node_publisher_d_cols->start; + +my $node_subscriber_d_cols = PostgreSQL::Test::Cluster->new('node_subscriber_d_cols'); +$node_subscriber_d_cols->init(allows_streaming => 'logical'); +$node_subscriber_d_cols->start; + +$node_publisher_d_cols->safe_psql( + 'postgres', qq( + CREATE TABLE dropped_cols (a int, b_drop int, c int); + ALTER TABLE dropped_cols REPLICA IDENTITY FULL; + CREATE PUBLICATION pub_dropped_cols FOR TABLE dropped_cols; + -- some initial data + INSERT INTO dropped_cols VALUES (1, 1, 1); +)); + +$node_subscriber_d_cols->safe_psql( + 'postgres', qq( + CREATE TABLE dropped_cols (a int, b_drop int, c int); +)); + +my $publisher_connstr_d_cols = $node_publisher_d_cols->connstr . ' dbname=postgres'; +$node_subscriber_d_cols->safe_psql('postgres', + "CREATE SUBSCRIPTION sub_dropped_cols CONNECTION '$publisher_connstr_d_cols' PUBLICATION pub_dropped_cols" +); +$node_subscriber_d_cols->wait_for_subscription_sync; + +$node_publisher_d_cols->safe_psql( + 'postgres', qq( + ALTER TABLE dropped_cols DROP COLUMN b_drop; +)); +$node_subscriber_d_cols->safe_psql( + 'postgres', qq( + ALTER TABLE dropped_cols DROP COLUMN b_drop; +)); + +$node_publisher_d_cols->safe_psql( + 'postgres', qq( + UPDATE dropped_cols SET a = 100; +)); +$node_publisher_d_cols->wait_for_catchup('sub_dropped_cols'); + +is($node_subscriber_d_cols->safe_psql('postgres', "SELECT count(*) FROM dropped_cols WHERE a = 100"), + qq(1), 'replication with RI FULL and dropped columns'); + +$node_publisher_d_cols->stop('fast'); +$node_subscriber_d_cols->stop('fast'); + done_testing(); -- 2.34.1 [application/octet-stream] v2-0001-Ignore-generated-columns.patch (2.9K, ../../CACawEhWkFwQ5Zuu9NOQiJtjnQY+At8JuhKQ6cJ7wEvWv+Vb=+g@mail.gmail.com/5-v2-0001-Ignore-generated-columns.patch) download | inline diff: From b84f38deac1b02f057bfc05ff868fbf4a621b18b Mon Sep 17 00:00:00 2001 From: Onder Kalaci <[email protected]> Date: Thu, 16 Mar 2023 18:18:37 +0300 Subject: [PATCH v1 2/2] Ignore generated columns when REPLICA IDENTITY FULL Generated columns are filled with NULL values on slot_store_data() but not on table_scan_getnextslot(). With this commit, we skip such columns while checking tuple equality. --- src/backend/executor/execReplication.c | 5 +++-- src/test/subscription/t/100_bugs.pl | 10 +++++++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index 7c8b0d2667..482806961a 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -290,10 +290,11 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2, TypeCacheEntry *typentry; /* - * Ignore dropped columns as the publisher doesn't send those + * Ignore dropped and generated columns as the publisher doesn't + * send those */ att = TupleDescAttr(slot1->tts_tupleDescriptor, attrnum); - if (att->attisdropped) + if (att->attisdropped || att->attgenerated) continue; /* diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl index 255746094c..96126428f3 100644 --- a/src/test/subscription/t/100_bugs.pl +++ b/src/test/subscription/t/100_bugs.pl @@ -387,14 +387,18 @@ $node_publisher_d_cols->safe_psql( 'postgres', qq( CREATE TABLE dropped_cols (a int, b_drop int, c int); ALTER TABLE dropped_cols REPLICA IDENTITY FULL; - CREATE PUBLICATION pub_dropped_cols FOR TABLE dropped_cols; + CREATE TABLE generated_cols (a int, b_gen int GENERATED ALWAYS AS (5 * a) STORED, c int); + ALTER TABLE generated_cols REPLICA IDENTITY FULL; + CREATE PUBLICATION pub_dropped_cols FOR TABLE dropped_cols, generated_cols; -- some initial data INSERT INTO dropped_cols VALUES (1, 1, 1); + INSERT INTO generated_cols (a,c) VALUES (1, 1); )); $node_subscriber_d_cols->safe_psql( 'postgres', qq( CREATE TABLE dropped_cols (a int, b_drop int, c int); + CREATE TABLE generated_cols (a int, b_gen int GENERATED ALWAYS AS (5 * a) STORED, c int); )); my $publisher_connstr_d_cols = $node_publisher_d_cols->connstr . ' dbname=postgres'; @@ -415,12 +419,16 @@ $node_subscriber_d_cols->safe_psql( $node_publisher_d_cols->safe_psql( 'postgres', qq( UPDATE dropped_cols SET a = 100; + UPDATE generated_cols SET a = 100; )); $node_publisher_d_cols->wait_for_catchup('sub_dropped_cols'); is($node_subscriber_d_cols->safe_psql('postgres', "SELECT count(*) FROM dropped_cols WHERE a = 100"), qq(1), 'replication with RI FULL and dropped columns'); +is($node_subscriber_d_cols->safe_psql('postgres', "SELECT count(*) FROM generated_cols WHERE a = 100"), + qq(1), 'replication with RI FULL and generated columns'); + $node_publisher_d_cols->stop('fast'); $node_subscriber_d_cols->stop('fast'); -- 2.34.1 ^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2023-03-17 07:38 UTC | newest] Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-03-12 20:43 [PATCH 07/10] add wal_compression_method: zstd Justin Pryzby <[email protected]> 2023-03-13 10:53 RE: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL [email protected] <[email protected]> 2023-03-13 12:56 ` Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL Önder Kalacı <[email protected]> 2023-03-16 09:23 ` Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL Amit Kapila <[email protected]> 2023-03-16 10:38 ` RE: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL [email protected] <[email protected]> 2023-03-16 16:03 ` Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL Önder Kalacı <[email protected]> 2023-03-17 00:08 ` Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL Amit Kapila <[email protected]> 2023-03-17 00:11 ` Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL Tom Lane <[email protected]> 2023-03-17 07:38 ` Re: Dropped and generated columns might cause wrong data on subs when REPLICA IDENTITY FULL Önder Kalacı <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox