public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v48 6/7] Remove the GUC stats_temp_directory 5+ messages / 4 participants [nested] [flat]
* [PATCH v48 6/7] Remove the GUC stats_temp_directory @ 2020-09-29 13:59 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Kyotaro Horiguchi @ 2020-09-29 13:59 UTC (permalink / raw) The new stats collection system doesn't need temporary directory, so just remove it. pg_stat_statements modified to use pg_stat directory to store its temporary files. As the result basebackup copies the pg_stat_statments' temporary file if exists. --- .../pg_stat_statements/pg_stat_statements.c | 13 ++++--- doc/src/sgml/backup.sgml | 2 -- doc/src/sgml/config.sgml | 19 ---------- doc/src/sgml/storage.sgml | 6 ---- src/backend/postmaster/pgstat.c | 10 ------ src/backend/replication/basebackup.c | 36 ------------------- src/backend/utils/misc/guc.c | 32 ----------------- src/backend/utils/misc/postgresql.conf.sample | 1 - src/bin/initdb/initdb.c | 1 - src/bin/pg_rewind/filemap.c | 7 ---- src/include/pgstat.h | 3 -- src/test/perl/PostgresNode.pm | 4 --- 12 files changed, 6 insertions(+), 128 deletions(-) diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 62cccbfa44..28279f97d5 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -89,14 +89,13 @@ PG_MODULE_MAGIC; #define PGSS_DUMP_FILE PGSTAT_STAT_PERMANENT_DIRECTORY "/pg_stat_statements.stat" /* - * Location of external query text file. We don't keep it in the core - * system's stats_temp_directory. The core system can safely use that GUC - * setting, because the statistics collector temp file paths are set only once - * as part of changing the GUC, but pg_stat_statements has no way of avoiding - * race conditions. Besides, we only expect modest, infrequent I/O for query - * strings, so placing the file on a faster filesystem is not compelling. + * Location of external query text file. We don't keep it in the core system's + * pg_stats. pg_stat_statements has no way of avoiding race conditions even if + * the directory were specified by a GUC. Besides, we only expect modest, + * infrequent I/O for query strings, so placing the file on a faster filesystem + * is not compelling. */ -#define PGSS_TEXT_FILE PG_STAT_TMP_DIR "/pgss_query_texts.stat" +#define PGSS_TEXT_FILE PGSTAT_STAT_PERMANENT_DIRECTORY "/pgss_query_texts.stat" /* Magic number identifying the stats file format */ static const uint32 PGSS_FILE_HEADER = 0x20201218; diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml index c5557d5444..875769a57e 100644 --- a/doc/src/sgml/backup.sgml +++ b/doc/src/sgml/backup.sgml @@ -1155,8 +1155,6 @@ SELECT pg_stop_backup(); <filename>pg_snapshots/</filename>, <filename>pg_stat_tmp/</filename>, and <filename>pg_subtrans/</filename> (but not the directories themselves) can be omitted from the backup as they will be initialized on postmaster startup. - If <xref linkend="guc-stats-temp-directory"/> is set and is under the data - directory then the contents of that directory can also be omitted. </para> <para> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 753b1ab6f9..49372c52d4 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -7488,25 +7488,6 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv; </listitem> </varlistentry> - <varlistentry id="guc-stats-temp-directory" xreflabel="stats_temp_directory"> - <term><varname>stats_temp_directory</varname> (<type>string</type>) - <indexterm> - <primary><varname>stats_temp_directory</varname> configuration parameter</primary> - </indexterm> - </term> - <listitem> - <para> - Sets the directory to store temporary statistics data in. This can be - a path relative to the data directory or an absolute path. The default - is <filename>pg_stat_tmp</filename>. Pointing this at a RAM-based - file system will decrease physical I/O requirements and can lead to - improved performance. - This parameter can only be set in the <filename>postgresql.conf</filename> - file or on the server command line. - </para> - </listitem> - </varlistentry> - </variablelist> </sect2> diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml index 3234adb639..6bac5e075e 100644 --- a/doc/src/sgml/storage.sgml +++ b/doc/src/sgml/storage.sgml @@ -120,12 +120,6 @@ Item subsystem</entry> </row> -<row> - <entry><filename>pg_stat_tmp</filename></entry> - <entry>Subdirectory containing temporary files for the statistics - subsystem</entry> -</row> - <row> <entry><filename>pg_subtrans</filename></entry> <entry>Subdirectory containing subtransaction status data</entry> diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index 68867c92b2..f62f06880e 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -99,16 +99,6 @@ bool pgstat_track_counts = false; int pgstat_track_functions = TRACK_FUNC_OFF; int pgstat_track_activity_query_size = 1024; -/* ---------- - * Built from GUC parameter - * ---------- - */ -char *pgstat_stat_directory = NULL; - -/* No longer used, but will be removed with GUC */ -char *pgstat_stat_filename = NULL; -char *pgstat_stat_tmpname = NULL; - /* * WAL usage counters saved from pgWALUsage at the previous call to * pgstat_send_wal(). This is used to calculate how much WAL usage diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c index fa7bdbcefa..f0d75f55bd 100644 --- a/src/backend/replication/basebackup.c +++ b/src/backend/replication/basebackup.c @@ -87,9 +87,6 @@ static int basebackup_read_file(int fd, char *buf, size_t nbytes, off_t offset, /* Was the backup currently in-progress initiated in recovery mode? */ static bool backup_started_in_recovery = false; -/* Relative path of temporary statistics directory */ -static char *statrelpath = NULL; - /* * Size of each block sent into the tar stream for larger files. */ @@ -152,13 +149,6 @@ struct exclude_list_item */ static const char *const excludeDirContents[] = { - /* - * Skip temporary statistics files. PG_STAT_TMP_DIR must be skipped even - * when stats_temp_directory is set because PGSS_TEXT_FILE is always - * created there. - */ - PG_STAT_TMP_DIR, - /* * It is generally not useful to backup the contents of this directory * even if the intention is to restore to another primary. See backup.sgml @@ -261,7 +251,6 @@ perform_base_backup(basebackup_options *opt) StringInfo labelfile; StringInfo tblspc_map_file; backup_manifest_info manifest; - int datadirpathlen; List *tablespaces = NIL; backup_total = 0; @@ -284,8 +273,6 @@ perform_base_backup(basebackup_options *opt) Assert(CurrentResourceOwner == NULL); CurrentResourceOwner = ResourceOwnerCreate(NULL, "base backup"); - datadirpathlen = strlen(DataDir); - backup_started_in_recovery = RecoveryInProgress(); labelfile = makeStringInfo(); @@ -314,18 +301,6 @@ perform_base_backup(basebackup_options *opt) tablespaceinfo *ti; int tblspc_streamed = 0; - /* - * Calculate the relative path of temporary statistics directory in - * order to skip the files which are located in that directory later. - */ - if (is_absolute_path(pgstat_stat_directory) && - strncmp(pgstat_stat_directory, DataDir, datadirpathlen) == 0) - statrelpath = psprintf("./%s", pgstat_stat_directory + datadirpathlen + 1); - else if (strncmp(pgstat_stat_directory, "./", 2) != 0) - statrelpath = psprintf("./%s", pgstat_stat_directory); - else - statrelpath = pgstat_stat_directory; - /* Add a node for the base directory at the end */ ti = palloc0(sizeof(tablespaceinfo)); ti->size = -1; @@ -1377,17 +1352,6 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces, if (excludeFound) continue; - /* - * Exclude contents of directory specified by statrelpath if not set - * to the default (pg_stat_tmp) which is caught in the loop above. - */ - if (statrelpath != NULL && strcmp(pathbuf, statrelpath) == 0) - { - elog(DEBUG1, "contents of directory \"%s\" excluded from backup", statrelpath); - size += _tarWriteDir(pathbuf, basepathlen, &statbuf, sizeonly); - continue; - } - /* * We can skip pg_wal, the WAL segments need to be fetched from the * WAL archive anyway. But include it as an empty directory anyway, so diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index c93dc63d98..4f500dbd06 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -203,7 +203,6 @@ static bool check_autovacuum_work_mem(int *newval, void **extra, GucSource sourc static bool check_effective_io_concurrency(int *newval, void **extra, GucSource source); static bool check_maintenance_io_concurrency(int *newval, void **extra, GucSource source); static bool check_huge_page_size(int *newval, void **extra, GucSource source); -static void assign_pgstat_temp_directory(const char *newval, void *extra); static bool check_application_name(char **newval, void **extra, GucSource source); static void assign_application_name(const char *newval, void *extra); static bool check_cluster_name(char **newval, void **extra, GucSource source); @@ -560,8 +559,6 @@ char *HbaFileName; char *IdentFileName; char *external_pid_file; -char *pgstat_temp_directory; - char *application_name; int tcp_keepalives_idle; @@ -11782,35 +11779,6 @@ check_huge_page_size(int *newval, void **extra, GucSource source) return true; } -static void -assign_pgstat_temp_directory(const char *newval, void *extra) -{ - /* check_canonical_path already canonicalized newval for us */ - char *dname; - char *tname; - char *fname; - - /* directory */ - dname = guc_malloc(ERROR, strlen(newval) + 1); /* runtime dir */ - sprintf(dname, "%s", newval); - - /* global stats */ - tname = guc_malloc(ERROR, strlen(newval) + 12); /* /global.tmp */ - sprintf(tname, "%s/global.tmp", newval); - fname = guc_malloc(ERROR, strlen(newval) + 13); /* /global.stat */ - sprintf(fname, "%s/global.stat", newval); - - if (pgstat_stat_directory) - free(pgstat_stat_directory); - pgstat_stat_directory = dname; - if (pgstat_stat_tmpname) - free(pgstat_stat_tmpname); - pgstat_stat_tmpname = tname; - if (pgstat_stat_filename) - free(pgstat_stat_filename); - pgstat_stat_filename = fname; -} - static bool check_application_name(char **newval, void **extra, GucSource source) { diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index 9640349a91..40a18bb248 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -588,7 +588,6 @@ #track_io_timing = off #track_functions = none # none, pl, all #track_activity_query_size = 1024 # (change requires restart) -#stats_temp_directory = 'pg_stat_tmp' # - Monitoring - diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index 3c1cf78b4f..07a00b8d0d 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -219,7 +219,6 @@ static const char *const subdirs[] = { "pg_replslot", "pg_tblspc", "pg_stat", - "pg_stat_tmp", "pg_xact", "pg_logical", "pg_logical/snapshots", diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c index 2618b4c957..ab5cb51de7 100644 --- a/src/bin/pg_rewind/filemap.c +++ b/src/bin/pg_rewind/filemap.c @@ -87,13 +87,6 @@ struct exclude_list_item */ static const char *excludeDirContents[] = { - /* - * Skip temporary statistics files. PG_STAT_TMP_DIR must be skipped even - * when stats_temp_directory is set because PGSS_TEXT_FILE is always - * created there. - */ - "pg_stat_tmp", /* defined as PG_STAT_TMP_DIR */ - /* * It is generally not useful to backup the contents of this directory * even if the intention is to restore to another primary. See backup.sgml diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 222104b88e..ce4feaea3b 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -33,9 +33,6 @@ #define PGSTAT_STAT_PERMANENT_FILENAME "pg_stat/saved_stats" #define PGSTAT_STAT_PERMANENT_TMPFILE "pg_stat/saved_stats.tmp" -/* Default directory to store temporary statistics data in */ -#define PG_STAT_TMP_DIR "pg_stat_tmp" - /* Values for track_functions GUC variable --- order is significant! */ typedef enum TrackFunctionsLevel { diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm index 9667f7667e..dd41a43b4e 100644 --- a/src/test/perl/PostgresNode.pm +++ b/src/test/perl/PostgresNode.pm @@ -455,10 +455,6 @@ sub init print $conf TestLib::slurp_file($ENV{TEMP_CONFIG}) if defined $ENV{TEMP_CONFIG}; - # XXX Neutralize any stats_temp_directory in TEMP_CONFIG. Nodes running - # concurrently must not share a stats_temp_directory. - print $conf "stats_temp_directory = 'pg_stat_tmp'\n"; - if ($params{allows_streaming}) { if ($params{allows_streaming} eq "logical") -- 2.27.0 ----Next_Part(Fri_Mar__5_17_18_56_2021_497)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v48-0007-Exclude-pg_stat-directory-from-base-backup.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: bogus: logical replication rows/cols combinations @ 2022-04-27 10:57 Alvaro Herrera <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Alvaro Herrera @ 2022-04-27 10:57 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Tomas Vondra <[email protected]>; Pg Hackers <[email protected]> On 2022-Apr-27, Amit Kapila wrote: > On Wed, Apr 27, 2022 at 3:13 PM Alvaro Herrera <[email protected]> wrote: > > > Changing this to behave the way you expect would be quite difficult, > > > because at the moment we build a single OR expression from all the row > > > filters. We'd have to keep the individual expressions, so that we can > > > build a column list for each of them (in order to ignore those that > > > don't match). > > > > I think we should do that, yeah. > > This can hit the performance as we need to evaluate each expression > for each row. So we do things because they are easy and fast, rather than because they work correctly? > > ... In fact I think they are quite orthogonal: probably you should be > > able to publish a partitioned table in two publications, with different > > rowfilters and different column lists (which can come from the > > topmost partitioned table), and each partition should still work in the > > way I describe above. > > We consider the column lists or row filters for either the partition > (on which the current operation is performed) or partitioned table > based on 'publish_via_partition_root' parameter of publication. OK, but this isn't relevant to what I wrote. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: bogus: logical replication rows/cols combinations @ 2022-04-27 11:48 Amit Kapila <[email protected]> parent: Alvaro Herrera <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Amit Kapila @ 2022-04-27 11:48 UTC (permalink / raw) To: Alvaro Herrera <[email protected]>; +Cc: Tomas Vondra <[email protected]>; Pg Hackers <[email protected]> On Wed, Apr 27, 2022 at 4:27 PM Alvaro Herrera <[email protected]> wrote: > > On 2022-Apr-27, Amit Kapila wrote: > > > On Wed, Apr 27, 2022 at 3:13 PM Alvaro Herrera <[email protected]> wrote: > > > > > Changing this to behave the way you expect would be quite difficult, > > > > because at the moment we build a single OR expression from all the row > > > > filters. We'd have to keep the individual expressions, so that we can > > > > build a column list for each of them (in order to ignore those that > > > > don't match). > > > > > > I think we should do that, yeah. > > > > This can hit the performance as we need to evaluate each expression > > for each row. > > So we do things because they are easy and fast, rather than because they > work correctly? > The point is I am not sure if what you are saying is better behavior than current but if others feel it is better then we can try to do something for it. In the above sentence, I just wanted to say that it will impact performance but if that is required then sure we should do it that way. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: bogus: logical replication rows/cols combinations @ 2022-04-27 21:56 Tomas Vondra <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Tomas Vondra @ 2022-04-27 21:56 UTC (permalink / raw) To: Amit Kapila <[email protected]>; Alvaro Herrera <[email protected]>; +Cc: Pg Hackers <[email protected]> Hi, so I've been looking at tweaking the code so that the behavior matches Alvaro's expectations. It passes check-world but I'm not claiming it's nowhere near commitable - the purpose is mostly to give better idea of how invasive the change is etc. As described earlier, this abandons the idea of building a single OR expression from all the row filters (per action), and replaces that with a list of per-publication info (struct PublicationInfo), combining info about both row filters and column lists. This means we can't initialize the row filters and column lists separately, but at the same time. So pgoutput_row_filter_init was modified to initialize both, and pgoutput_column_list_init was removed. With this info, we can calculate column lists only for publications with matching row filters, which is what the modified pgoutput_row_filter does (the calculated column list is returned through a parameter). This however does not remove the 'columns' from RelationSyncEntry entirely. We still need that "superset" column list when sending schema. Imagine two publications, one replicating (a,b) and the other (a,c), maybe depending on row filter. send_relation_and_attrs() needs to send info about all three attributes (a,b,c), i.e. about any attribute that might end up being replicated. We might try to be smarter and send the exact schema needed by the next operation, i.e. when inserting (a,b) we'd make sure the last schema we sent was (a,b) and invalidate/resend it otherwise. But that might easily result in "trashing" where we send the schema and the next operation invalidates it right away because it needs a different schema. But there's another reason to do it like this - it seems desirable to actually reset columns don't match the calculated column list. Using Alvaro's example, it seems reasonable to expect these two transactions to produce the same result on the subscriber: 1) insert (a,b) + update to (a,c) insert into uno values (1, 2, 3); update uno set a = -1 where a = 1; 2) insert (a,c) insert into uno values (-1, 2, 3); But to do this, the update actually needs to send (-1,NULL,3). So in this case we'll have (a,b,c) column list in RelationSyncEntry, and only attributes on this list will be sent as part of schema. And DML actions we'll calculate either (a,b) or (a,c) depending on the row filter, and missing attributes will be replicated as NULL. I haven't done any tests how this affect performance, but I have a couple thoughts regarding that: a) I kinda doubt the optimizations would really matter in practice, because how likely is it that one relation is in many publications (in the same subscription)? b) Did anyone actually do some benchmarks that I could repeat, to see how much worse this is? c) AFAICS we could optimize this in at least some common cases. For example we could combine the entries with matching row filters, and/or column filters. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company Attachments: [text/x-patch] rework-column-row-filtering.patch (30.4K, ../../[email protected]/2-rework-column-row-filtering.patch) download | inline diff: diff --git a/src/backend/replication/logical/proto.c b/src/backend/replication/logical/proto.c index ff8513e2d2..41c5e3413f 100644 --- a/src/backend/replication/logical/proto.c +++ b/src/backend/replication/logical/proto.c @@ -33,7 +33,9 @@ static void logicalrep_write_attrs(StringInfo out, Relation rel, Bitmapset *columns); static void logicalrep_write_tuple(StringInfo out, Relation rel, TupleTableSlot *slot, - bool binary, Bitmapset *columns); + bool binary, + Bitmapset *schema_columns, + Bitmapset *columns); static void logicalrep_read_attrs(StringInfo in, LogicalRepRelation *rel); static void logicalrep_read_tuple(StringInfo in, LogicalRepTupleData *tuple); @@ -412,7 +414,8 @@ logicalrep_read_origin(StringInfo in, XLogRecPtr *origin_lsn) */ void logicalrep_write_insert(StringInfo out, TransactionId xid, Relation rel, - TupleTableSlot *newslot, bool binary, Bitmapset *columns) + TupleTableSlot *newslot, bool binary, + Bitmapset *schema_columns, Bitmapset *columns) { pq_sendbyte(out, LOGICAL_REP_MSG_INSERT); @@ -424,7 +427,8 @@ logicalrep_write_insert(StringInfo out, TransactionId xid, Relation rel, pq_sendint32(out, RelationGetRelid(rel)); pq_sendbyte(out, 'N'); /* new tuple follows */ - logicalrep_write_tuple(out, rel, newslot, binary, columns); + logicalrep_write_tuple(out, rel, newslot, binary, + schema_columns, columns); } /* @@ -457,7 +461,8 @@ logicalrep_read_insert(StringInfo in, LogicalRepTupleData *newtup) void logicalrep_write_update(StringInfo out, TransactionId xid, Relation rel, TupleTableSlot *oldslot, TupleTableSlot *newslot, - bool binary, Bitmapset *columns) + bool binary, Bitmapset *schema_columns, + Bitmapset *columns) { pq_sendbyte(out, LOGICAL_REP_MSG_UPDATE); @@ -478,11 +483,12 @@ logicalrep_write_update(StringInfo out, TransactionId xid, Relation rel, pq_sendbyte(out, 'O'); /* old tuple follows */ else pq_sendbyte(out, 'K'); /* old key follows */ - logicalrep_write_tuple(out, rel, oldslot, binary, NULL); + logicalrep_write_tuple(out, rel, oldslot, binary, NULL, NULL); } pq_sendbyte(out, 'N'); /* new tuple follows */ - logicalrep_write_tuple(out, rel, newslot, binary, columns); + logicalrep_write_tuple(out, rel, newslot, binary, + schema_columns, columns); } /* @@ -551,7 +557,7 @@ logicalrep_write_delete(StringInfo out, TransactionId xid, Relation rel, else pq_sendbyte(out, 'K'); /* old key follows */ - logicalrep_write_tuple(out, rel, oldslot, binary, NULL); + logicalrep_write_tuple(out, rel, oldslot, binary, NULL, NULL); } /* @@ -766,7 +772,8 @@ logicalrep_read_typ(StringInfo in, LogicalRepTyp *ltyp) */ static void logicalrep_write_tuple(StringInfo out, Relation rel, TupleTableSlot *slot, - bool binary, Bitmapset *columns) + bool binary, + Bitmapset *schema_columns, Bitmapset *columns) { TupleDesc desc; Datum *values; @@ -783,7 +790,7 @@ logicalrep_write_tuple(StringInfo out, Relation rel, TupleTableSlot *slot, if (att->attisdropped || att->attgenerated) continue; - if (!column_in_column_list(att->attnum, columns)) + if (!column_in_column_list(att->attnum, schema_columns)) continue; nliveatts++; @@ -804,10 +811,23 @@ logicalrep_write_tuple(StringInfo out, Relation rel, TupleTableSlot *slot, if (att->attisdropped || att->attgenerated) continue; - if (!column_in_column_list(att->attnum, columns)) + /* + * Columns that are not in schema (union of column lists) should + * be skipped entirely. + */ + if (!column_in_column_list(att->attnum, schema_columns)) continue; - if (isnull[i]) + /* + * Columns not in the column list (derived consindering row filters) + * we just send NULL. + * + * XXX Not sure this is quite correct, though. Imagine you replicate + * values for columns (A,B), but it changes the row filter. Can we + * send NULL that would overwrite "proper" value replicated earlier? + */ + if (isnull[i] || + !column_in_column_list(att->attnum, columns)) { pq_sendbyte(out, LOGICALREP_COLUMN_NULL); continue; diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c index b197bfd565..85456e6d9f 100644 --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -137,13 +137,13 @@ typedef struct RelationSyncEntry PublicationActions pubactions; /* - * ExprState array for row filter. Different publication actions don't - * allow multiple expressions to always be combined into one, because - * updates or deletes restrict the column in expression to be part of the - * replica identity index whereas inserts do not have this restriction, so - * there is one ExprState per publication action. + * Info about row filters and column lists for each publication this + * relation is included in. We keep a list with per-publication info in + * order to calculate appropriate column list depending on which row + * filter(s) match. Each element contains an ExprState for the filter + * and column list. */ - ExprState *exprstate[NUM_ROWFILTER_PUBACTIONS]; + dlist_head pubinfos; /* one per publication */ EState *estate; /* executor state used for row filter */ TupleTableSlot *new_slot; /* slot for storing new tuple */ TupleTableSlot *old_slot; /* slot for storing old tuple */ @@ -208,6 +208,29 @@ typedef struct PGOutputTxnData * been sent */ } PGOutputTxnData; +/* + * Info use to track and evaluate row filters for each publication the relation + * is included in, and calculat ethe column list. + */ +typedef struct PublicationInfo { + + /* doubly-linked list */ + dlist_node node; + + /* publication OID (XXX not really needed) */ + Oid oid; + + /* row filter (expression state) */ + ExprState *rowfilter; + + /* column list */ + Bitmapset *columns; + + /* actions published by the publication */ + PublicationActions pubactions; + +} PublicationInfo; + /* Map used to remember which relation schemas we sent. */ static HTAB *RelationSyncCache = NULL; @@ -235,12 +258,8 @@ static bool pgoutput_row_filter_exec_expr(ExprState *state, static bool pgoutput_row_filter(Relation relation, TupleTableSlot *old_slot, TupleTableSlot **new_slot_ptr, RelationSyncEntry *entry, - ReorderBufferChangeType *action); - -/* column list routines */ -static void pgoutput_column_list_init(PGOutputData *data, - List *publications, - RelationSyncEntry *entry); + ReorderBufferChangeType *action, + Bitmapset **column_list); /* * Specify output plugin callbacks @@ -822,18 +841,24 @@ pgoutput_ensure_entry_cxt(PGOutputData *data, RelationSyncEntry *entry) } /* - * Initialize the row filter. + * Initialize the row filter and column list. + * + * Prepare information (ExprState, etc) used to evaluate per-publication row + * filters, and column lists. + * + * We also calculate a "total" column list as a union of all per-publication + * column lists, irrespectedly of row filters. This is used to send schema + * for the relsync entry, etc. */ static void pgoutput_row_filter_init(PGOutputData *data, List *publications, RelationSyncEntry *entry) { ListCell *lc; - List *rfnodes[] = {NIL, NIL, NIL}; /* One per pubaction */ - bool no_filter[] = {false, false, false}; /* One per pubaction */ MemoryContext oldctx; - int idx; - bool has_filter = true; + bool all_columns = false; + + dlist_init(&entry->pubinfos); /* * Find if there are any row filters for this relation. If there are, then @@ -855,7 +880,12 @@ pgoutput_row_filter_init(PGOutputData *data, List *publications, Publication *pub = lfirst(lc); HeapTuple rftuple = NULL; Datum rfdatum = 0; + Datum cfdatum = 0; bool pub_no_filter = false; + bool pub_no_list = false; + Relation relation = RelationIdGetRelation(entry->publish_as_relid); + + PublicationInfo *pubinfo = NULL; if (pub->alltables) { @@ -865,6 +895,7 @@ pgoutput_row_filter_init(PGOutputData *data, List *publications, * publications it does). */ pub_no_filter = true; + pub_no_list = true; } else { @@ -881,191 +912,75 @@ pgoutput_row_filter_init(PGOutputData *data, List *publications, rfdatum = SysCacheGetAttr(PUBLICATIONRELMAP, rftuple, Anum_pg_publication_rel_prqual, &pub_no_filter); + + /* + * Lookup the column list attribute. + * + * Null indicates no list. + */ + cfdatum = SysCacheGetAttr(PUBLICATIONRELMAP, rftuple, + Anum_pg_publication_rel_prattrs, + &pub_no_list); } else { pub_no_filter = true; + pub_no_list = true; } } - if (pub_no_filter) - { - if (rftuple) - ReleaseSysCache(rftuple); - - no_filter[PUBACTION_INSERT] |= pub->pubactions.pubinsert; - no_filter[PUBACTION_UPDATE] |= pub->pubactions.pubupdate; - no_filter[PUBACTION_DELETE] |= pub->pubactions.pubdelete; - - /* - * Quick exit if all the DML actions are publicized via this - * publication. - */ - if (no_filter[PUBACTION_INSERT] && - no_filter[PUBACTION_UPDATE] && - no_filter[PUBACTION_DELETE]) - { - has_filter = false; - break; - } - - /* No additional work for this publication. Next one. */ - continue; - } - - /* Form the per pubaction row filter lists. */ - if (pub->pubactions.pubinsert && !no_filter[PUBACTION_INSERT]) - rfnodes[PUBACTION_INSERT] = lappend(rfnodes[PUBACTION_INSERT], - TextDatumGetCString(rfdatum)); - if (pub->pubactions.pubupdate && !no_filter[PUBACTION_UPDATE]) - rfnodes[PUBACTION_UPDATE] = lappend(rfnodes[PUBACTION_UPDATE], - TextDatumGetCString(rfdatum)); - if (pub->pubactions.pubdelete && !no_filter[PUBACTION_DELETE]) - rfnodes[PUBACTION_DELETE] = lappend(rfnodes[PUBACTION_DELETE], - TextDatumGetCString(rfdatum)); - - ReleaseSysCache(rftuple); - } /* loop all subscribed publications */ - - /* Clean the row filter */ - for (idx = 0; idx < NUM_ROWFILTER_PUBACTIONS; idx++) - { - if (no_filter[idx]) - { - list_free_deep(rfnodes[idx]); - rfnodes[idx] = NIL; - } - } - - if (has_filter) - { - Relation relation = RelationIdGetRelation(entry->publish_as_relid); - pgoutput_ensure_entry_cxt(data, entry); - /* - * Now all the filters for all pubactions are known. Combine them when - * their pubactions are the same. - */ oldctx = MemoryContextSwitchTo(entry->entry_cxt); - entry->estate = create_estate_for_relation(relation); - for (idx = 0; idx < NUM_ROWFILTER_PUBACTIONS; idx++) - { - List *filters = NIL; - Expr *rfnode; - if (rfnodes[idx] == NIL) - continue; - - foreach(lc, rfnodes[idx]) - filters = lappend(filters, stringToNode((char *) lfirst(lc))); - - /* combine the row filter and cache the ExprState */ - rfnode = make_orclause(filters); - entry->exprstate[idx] = ExecPrepareExpr(rfnode, entry->estate); - } /* for each pubaction */ - MemoryContextSwitchTo(oldctx); - - RelationClose(relation); - } -} + pubinfo = (PublicationInfo *) palloc0(sizeof(PublicationInfo)); -/* - * Initialize the column list. - */ -static void -pgoutput_column_list_init(PGOutputData *data, List *publications, - RelationSyncEntry *entry) -{ - ListCell *lc; + pubinfo->oid = pub->oid; + pubinfo->pubactions = pub->pubactions; - /* - * Find if there are any column lists for this relation. If there are, - * build a bitmap merging all the column lists. - * - * All the given publication-table mappings must be checked. - * - * Multiple publications might have multiple column lists for this relation. - * - * FOR ALL TABLES and FOR ALL TABLES IN SCHEMA implies "don't use column - * list" so it takes precedence. - */ - foreach(lc, publications) - { - Publication *pub = lfirst(lc); - HeapTuple cftuple = NULL; - Datum cfdatum = 0; + if (!pub_no_filter) + { + entry->estate = create_estate_for_relation(relation); - /* - * Assume there's no column list. Only if we find pg_publication_rel - * entry with a column list we'll switch it to false. - */ - bool pub_no_list = true; + pubinfo->rowfilter + = ExecPrepareExpr(stringToNode(TextDatumGetCString(rfdatum)), + entry->estate); + } /* - * If the publication is FOR ALL TABLES then it is treated the same as if - * there are no column lists (even if other publications have a list). + * Build the column list bitmap in the per-entry context. + * + * We need to merge column lists from all publications, so we + * update the same bitmapset. If the column list is null, we + * interpret it as replicating all columns. */ - if (!pub->alltables) + pubinfo->columns = NULL; + if (!pub_no_list) /* when not null */ { - /* - * Check for the presence of a column list in this publication. - * - * Note: If we find no pg_publication_rel row, it's a publication - * defined for a whole schema, so it can't have a column list, just - * like a FOR ALL TABLES publication. - */ - cftuple = SearchSysCache2(PUBLICATIONRELMAP, - ObjectIdGetDatum(entry->publish_as_relid), - ObjectIdGetDatum(pub->oid)); - - if (HeapTupleIsValid(cftuple)) - { - /* - * Lookup the column list attribute. - * - * Note: We update the pub_no_list value directly, because if - * the value is NULL, we have no list (and vice versa). - */ - cfdatum = SysCacheGetAttr(PUBLICATIONRELMAP, cftuple, - Anum_pg_publication_rel_prattrs, - &pub_no_list); - - /* - * Build the column list bitmap in the per-entry context. - * - * We need to merge column lists from all publications, so we - * update the same bitmapset. If the column list is null, we - * interpret it as replicating all columns. - */ - if (!pub_no_list) /* when not null */ - { - pgoutput_ensure_entry_cxt(data, entry); + pubinfo->columns = pub_collist_to_bitmapset(pubinfo->columns, + cfdatum, + entry->entry_cxt); - entry->columns = pub_collist_to_bitmapset(entry->columns, - cfdatum, - entry->entry_cxt); - } - } + entry->columns = pub_collist_to_bitmapset(entry->columns, + cfdatum, + entry->entry_cxt); } + else + all_columns = true; - /* - * Found a publication with no column list, so we're done. But first - * discard column list we might have from preceding publications. - */ - if (pub_no_list) - { - if (cftuple) - ReleaseSysCache(cftuple); + if (HeapTupleIsValid(rftuple)) + ReleaseSysCache(rftuple); - bms_free(entry->columns); - entry->columns = NULL; + MemoryContextSwitchTo(oldctx); + RelationClose(relation); - break; - } + dlist_push_tail(&entry->pubinfos, &pubinfo->node); - ReleaseSysCache(cftuple); } /* loop all subscribed publications */ + + /* any of the publications replicates all columns */ + if (all_columns) + entry->columns = NULL; } /* @@ -1115,7 +1030,8 @@ init_tuple_slot(PGOutputData *data, Relation relation, } /* - * Change is checked against the row filter if any. + * Change is checked against the row filter if any, and calculate the column + * list applicable to the operation (with respect to matching row filters). * * Returns true if the change is to be replicated, else false. * @@ -1136,6 +1052,8 @@ init_tuple_slot(PGOutputData *data, Relation relation, * * The new action is updated in the action parameter. * + * The calculated column list is returned in the column_list parameter. + * * The new slot could be updated when transforming the UPDATE into INSERT, * because the original new tuple might not have column values from the replica * identity. @@ -1167,17 +1085,21 @@ init_tuple_slot(PGOutputData *data, Relation relation, static bool pgoutput_row_filter(Relation relation, TupleTableSlot *old_slot, TupleTableSlot **new_slot_ptr, RelationSyncEntry *entry, - ReorderBufferChangeType *action) + ReorderBufferChangeType *action, Bitmapset **column_list) { TupleDesc desc; int i; - bool old_matched, - new_matched, + bool old_matched_any = false, + new_matched_any = false, result; - TupleTableSlot *tmp_new_slot; + TupleTableSlot *tmp_new_slot = NULL; TupleTableSlot *new_slot = *new_slot_ptr; - ExprContext *ecxt; - ExprState *filter_exprstate; + dlist_iter iter; + bool matching = false; + + /* Column list calculated from publications matching the row filter. */ + Bitmapset *columns = NULL; + bool all_columns = false; /* * We need this map to avoid relying on ReorderBufferChangeType enums @@ -1195,115 +1117,191 @@ pgoutput_row_filter(Relation relation, TupleTableSlot *old_slot, Assert(new_slot || old_slot); - /* Get the corresponding row filter */ - filter_exprstate = entry->exprstate[map_changetype_pubaction[*action]]; + dlist_foreach(iter, &entry->pubinfos) + { + ExprContext *ecxt; + bool old_matched, + new_matched; - /* Bail out if there is no row filter */ - if (!filter_exprstate) - return true; + PublicationInfo *pubinfo + = dlist_container(PublicationInfo, node, iter.cur); - elog(DEBUG3, "table \"%s.%s\" has row filter", - get_namespace_name(RelationGetNamespace(relation)), - RelationGetRelationName(relation)); + /* ignore publications not replicating this action */ + if ((*action == REORDER_BUFFER_CHANGE_INSERT) && + (!pubinfo->pubactions.pubinsert)) + continue; + else if ((*action == REORDER_BUFFER_CHANGE_UPDATE) && + (!pubinfo->pubactions.pubupdate)) + continue; + else if ((*action == REORDER_BUFFER_CHANGE_DELETE) && + (!pubinfo->pubactions.pubdelete)) + continue; - ResetPerTupleExprContext(entry->estate); + if (!pubinfo->rowfilter) + { + matching = true; - ecxt = GetPerTupleExprContext(entry->estate); + /* + * Update/merge the column list. + * + * If the publication has no column list, we interpret it as a list + * with all columns. Otherwise we just add it to the bitmap. + * + * FIXME This is repeated in three places. Maybe refactor? + */ + if (!pubinfo->columns) + { + all_columns = true; + bms_free(columns); + columns = NULL; + } + else if (!all_columns) + columns = bms_union(columns, pubinfo->columns); - /* - * For the following occasions where there is only one tuple, we can - * evaluate the row filter for that tuple and return. - * - * For inserts, we only have the new tuple. - * - * For updates, we can have only a new tuple when none of the replica - * identity columns changed and none of those columns have external data - * but we still need to evaluate the row filter for the new tuple as the - * existing values of those columns might not match the filter. Also, users - * can use constant expressions in the row filter, so we anyway need to - * evaluate it for the new tuple. - * - * For deletes, we only have the old tuple. - */ - if (!new_slot || !old_slot) - { - ecxt->ecxt_scantuple = new_slot ? new_slot : old_slot; - result = pgoutput_row_filter_exec_expr(filter_exprstate, ecxt); + continue; + } - return result; - } + elog(DEBUG3, "table \"%s.%s\" has row filter", + get_namespace_name(RelationGetNamespace(relation)), + RelationGetRelationName(relation)); - /* - * Both the old and new tuples must be valid only for updates and need to - * be checked against the row filter. - */ - Assert(map_changetype_pubaction[*action] == PUBACTION_UPDATE); + ResetPerTupleExprContext(entry->estate); + + ecxt = GetPerTupleExprContext(entry->estate); - slot_getallattrs(new_slot); - slot_getallattrs(old_slot); + /* + * For the following occasions where there is only one tuple, we can + * evaluate the row filter for that tuple and return. + * + * For inserts, we only have the new tuple. + * + * For updates, we can have only a new tuple when none of the replica + * identity columns changed and none of those columns have external data + * but we still need to evaluate the row filter for the new tuple as the + * existing values of those columns might not match the filter. Also, users + * can use constant expressions in the row filter, so we anyway need to + * evaluate it for the new tuple. + * + * For deletes, we only have the old tuple. + */ + if (!new_slot || !old_slot) + { + ecxt->ecxt_scantuple = new_slot ? new_slot : old_slot; + result = pgoutput_row_filter_exec_expr(pubinfo->rowfilter, ecxt); - tmp_new_slot = NULL; - desc = RelationGetDescr(relation); + matching |= result; - /* - * The new tuple might not have all the replica identity columns, in which - * case it needs to be copied over from the old tuple. - */ - for (i = 0; i < desc->natts; i++) - { - Form_pg_attribute att = TupleDescAttr(desc, i); + /* + * FIXME refactor to reuse this code in multiple places + * + * XXX Should this only update column list when using new slot? + * If evaluationg old slot, that's delete, no? + */ + if (result) + { + if (!pubinfo->columns) + { + all_columns = true; + bms_free(columns); + columns = NULL; + } + else if (!all_columns) + { + columns = bms_union(columns, pubinfo->columns); + } + } + + continue; + } /* - * if the column in the new tuple or old tuple is null, nothing to do + * Both the old and new tuples must be valid only for updates and need to + * be checked against the row filter. */ - if (new_slot->tts_isnull[i] || old_slot->tts_isnull[i]) - continue; + Assert(map_changetype_pubaction[*action] == PUBACTION_UPDATE); + + slot_getallattrs(new_slot); + slot_getallattrs(old_slot); + + tmp_new_slot = NULL; + desc = RelationGetDescr(relation); /* - * Unchanged toasted replica identity columns are only logged in the - * old tuple. Copy this over to the new tuple. The changed (or WAL - * Logged) toast values are always assembled in memory and set as - * VARTAG_INDIRECT. See ReorderBufferToastReplace. + * The new tuple might not have all the replica identity columns, in which + * case it needs to be copied over from the old tuple. */ - if (att->attlen == -1 && - VARATT_IS_EXTERNAL_ONDISK(new_slot->tts_values[i]) && - !VARATT_IS_EXTERNAL_ONDISK(old_slot->tts_values[i])) + for (i = 0; i < desc->natts; i++) { - if (!tmp_new_slot) + Form_pg_attribute att = TupleDescAttr(desc, i); + + /* + * if the column in the new tuple or old tuple is null, nothing to do + */ + if (new_slot->tts_isnull[i] || old_slot->tts_isnull[i]) + continue; + + /* + * Unchanged toasted replica identity columns are only logged in the + * old tuple. Copy this over to the new tuple. The changed (or WAL + * Logged) toast values are always assembled in memory and set as + * VARTAG_INDIRECT. See ReorderBufferToastReplace. + */ + if (att->attlen == -1 && + VARATT_IS_EXTERNAL_ONDISK(new_slot->tts_values[i]) && + !VARATT_IS_EXTERNAL_ONDISK(old_slot->tts_values[i])) { - tmp_new_slot = MakeSingleTupleTableSlot(desc, &TTSOpsVirtual); - ExecClearTuple(tmp_new_slot); + if (!tmp_new_slot) + { + tmp_new_slot = MakeSingleTupleTableSlot(desc, &TTSOpsVirtual); + ExecClearTuple(tmp_new_slot); - memcpy(tmp_new_slot->tts_values, new_slot->tts_values, - desc->natts * sizeof(Datum)); - memcpy(tmp_new_slot->tts_isnull, new_slot->tts_isnull, - desc->natts * sizeof(bool)); - } + memcpy(tmp_new_slot->tts_values, new_slot->tts_values, + desc->natts * sizeof(Datum)); + memcpy(tmp_new_slot->tts_isnull, new_slot->tts_isnull, + desc->natts * sizeof(bool)); + } - tmp_new_slot->tts_values[i] = old_slot->tts_values[i]; - tmp_new_slot->tts_isnull[i] = old_slot->tts_isnull[i]; + tmp_new_slot->tts_values[i] = old_slot->tts_values[i]; + tmp_new_slot->tts_isnull[i] = old_slot->tts_isnull[i]; + } } - } - ecxt->ecxt_scantuple = old_slot; - old_matched = pgoutput_row_filter_exec_expr(filter_exprstate, ecxt); + ecxt->ecxt_scantuple = old_slot; - if (tmp_new_slot) - { - ExecStoreVirtualTuple(tmp_new_slot); - ecxt->ecxt_scantuple = tmp_new_slot; - } - else - ecxt->ecxt_scantuple = new_slot; + old_matched = pgoutput_row_filter_exec_expr(pubinfo->rowfilter, ecxt); + old_matched_any |= old_matched; + + if (tmp_new_slot) + { + ExecStoreVirtualTuple(tmp_new_slot); + ecxt->ecxt_scantuple = tmp_new_slot; + } + else + ecxt->ecxt_scantuple = new_slot; - new_matched = pgoutput_row_filter_exec_expr(filter_exprstate, ecxt); + new_matched = pgoutput_row_filter_exec_expr(pubinfo->rowfilter, ecxt); + new_matched_any |= new_matched; - /* - * Case 1: if both tuples don't match the row filter, bailout. Send - * nothing. - */ - if (!old_matched && !new_matched) - return false; + /* + * Case 1: if both tuples don't match the row filter, bailout. Send + * nothing. + */ + if (!old_matched && !new_matched) + continue; /* continue with the next row filter */ + + /* + * Case 4: if both tuples match the row filter, transformation isn't + * required. (*action is default UPDATE). + */ + if (!pubinfo->columns) + { + all_columns = true; + bms_free(columns); + columns = NULL; + } + else if (!all_columns && new_matched) + columns = bms_union(columns, pubinfo->columns); + } /* * Case 2: if the old tuple doesn't satisfy the row filter but the new @@ -1314,9 +1312,10 @@ pgoutput_row_filter(Relation relation, TupleTableSlot *old_slot, * while inserting the tuple in the downstream node, we have all the * required column values. */ - if (!old_matched && new_matched) + if (!old_matched_any && new_matched_any) { *action = REORDER_BUFFER_CHANGE_INSERT; + matching = true; if (tmp_new_slot) *new_slot_ptr = tmp_new_slot; @@ -1329,15 +1328,18 @@ pgoutput_row_filter(Relation relation, TupleTableSlot *old_slot, * This transformation does not require another tuple. The Old tuple will * be used for DELETE. */ - else if (old_matched && !new_matched) + else if (old_matched_any && !new_matched_any) + { *action = REORDER_BUFFER_CHANGE_DELETE; + matching = true; + } + else if (old_matched_any && new_matched_any) + matching = true; - /* - * Case 4: if both tuples match the row filter, transformation isn't - * required. (*action is default UPDATE). - */ + if (column_list) + *column_list = columns; - return true; + return matching; } /* @@ -1359,6 +1361,7 @@ pgoutput_change(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, ReorderBufferChangeType action = change->action; TupleTableSlot *old_slot = NULL; TupleTableSlot *new_slot = NULL; + Bitmapset *columns = NULL; if (!is_publishable_relation(relation)) return; @@ -1423,7 +1426,7 @@ pgoutput_change(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, /* Check row filter */ if (!pgoutput_row_filter(targetrel, NULL, &new_slot, relentry, - &action)) + &action, &columns)) break; /* @@ -1444,7 +1447,8 @@ pgoutput_change(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, OutputPluginPrepareWrite(ctx, true); logicalrep_write_insert(ctx->out, xid, targetrel, new_slot, - data->binary, relentry->columns); + data->binary, + relentry->columns, columns); OutputPluginWrite(ctx, true); break; case REORDER_BUFFER_CHANGE_UPDATE: @@ -1483,7 +1487,7 @@ pgoutput_change(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, /* Check row filter */ if (!pgoutput_row_filter(targetrel, old_slot, &new_slot, - relentry, &action)) + relentry, &action, &columns)) break; /* Send BEGIN if we haven't yet */ @@ -1503,12 +1507,12 @@ pgoutput_change(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, case REORDER_BUFFER_CHANGE_INSERT: logicalrep_write_insert(ctx->out, xid, targetrel, new_slot, data->binary, - relentry->columns); + relentry->columns, columns); break; case REORDER_BUFFER_CHANGE_UPDATE: logicalrep_write_update(ctx->out, xid, targetrel, old_slot, new_slot, data->binary, - relentry->columns); + relentry->columns, columns); break; case REORDER_BUFFER_CHANGE_DELETE: logicalrep_write_delete(ctx->out, xid, targetrel, @@ -1547,7 +1551,7 @@ pgoutput_change(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, /* Check row filter */ if (!pgoutput_row_filter(targetrel, old_slot, &new_slot, - relentry, &action)) + relentry, &action, NULL)) break; /* Send BEGIN if we haven't yet */ @@ -1977,7 +1981,7 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) entry->pubactions.pubdelete = entry->pubactions.pubtruncate = false; entry->new_slot = NULL; entry->old_slot = NULL; - memset(entry->exprstate, 0, sizeof(entry->exprstate)); + dlist_init(&entry->pubinfos); entry->entry_cxt = NULL; entry->publish_as_relid = InvalidOid; entry->columns = NULL; @@ -2056,7 +2060,7 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) entry->entry_cxt = NULL; entry->estate = NULL; - memset(entry->exprstate, 0, sizeof(entry->exprstate)); + dlist_init(&entry->pubinfos); /* * Build publication cache. We can't use one provided by relcache as @@ -2192,11 +2196,8 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) /* Initialize the tuple slot and map */ init_tuple_slot(data, relation, entry); - /* Initialize the row filter */ + /* Initialize the row filter and column list info */ pgoutput_row_filter_init(data, rel_publications, entry); - - /* Initialize the column list */ - pgoutput_column_list_init(data, rel_publications, entry); } list_free(pubids); diff --git a/src/include/replication/logicalproto.h b/src/include/replication/logicalproto.h index a771ab8ff3..7d45b94d3c 100644 --- a/src/include/replication/logicalproto.h +++ b/src/include/replication/logicalproto.h @@ -209,12 +209,14 @@ extern char *logicalrep_read_origin(StringInfo in, XLogRecPtr *origin_lsn); extern void logicalrep_write_insert(StringInfo out, TransactionId xid, Relation rel, TupleTableSlot *newslot, - bool binary, Bitmapset *columns); + bool binary, Bitmapset *schema_columns, + Bitmapset *columns); extern LogicalRepRelId logicalrep_read_insert(StringInfo in, LogicalRepTupleData *newtup); extern void logicalrep_write_update(StringInfo out, TransactionId xid, Relation rel, TupleTableSlot *oldslot, - TupleTableSlot *newslot, bool binary, Bitmapset *columns); + TupleTableSlot *newslot, bool binary, + Bitmapset *schema_columns, Bitmapset *columns); extern LogicalRepRelId logicalrep_read_update(StringInfo in, bool *has_oldtuple, LogicalRepTupleData *oldtup, LogicalRepTupleData *newtup); ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: bogus: logical replication rows/cols combinations @ 2022-04-28 03:17 Amit Kapila <[email protected]> parent: Tomas Vondra <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Amit Kapila @ 2022-04-28 03:17 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Pg Hackers <[email protected]> On Thu, Apr 28, 2022 at 3:26 AM Tomas Vondra <[email protected]> wrote: > > so I've been looking at tweaking the code so that the behavior matches > Alvaro's expectations. It passes check-world but I'm not claiming it's > nowhere near commitable - the purpose is mostly to give better idea of > how invasive the change is etc. > I was just skimming through the patch and didn't find anything related to initial sync handling. I feel the behavior should be same for initial sync and replication. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2022-04-28 03:17 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-09-29 13:59 [PATCH v48 6/7] Remove the GUC stats_temp_directory Kyotaro Horiguchi <[email protected]> 2022-04-27 10:57 Re: bogus: logical replication rows/cols combinations Alvaro Herrera <[email protected]> 2022-04-27 11:48 ` Re: bogus: logical replication rows/cols combinations Amit Kapila <[email protected]> 2022-04-27 21:56 ` Re: bogus: logical replication rows/cols combinations Tomas Vondra <[email protected]> 2022-04-28 03:17 ` Re: bogus: logical replication rows/cols combinations Amit Kapila <[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