public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v45 6/7] Remove the GUC stats_temp_directory
6+ messages / 4 participants
[nested] [flat]
* [PATCH v45 6/7] Remove the GUC stats_temp_directory
@ 2020-09-29 13:59 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 6+ 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 | 43 -------------------
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(+), 139 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 72a117fc19..0a98b2f2c0 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 3c8aaed0b6..7557a375f0 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1146,8 +1146,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 f6c80df988..906f893891 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7440,25 +7440,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 ecf9d9adcc..73b44a2652 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 d21801cf90..d2c3064678 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 85299e2138..16e430fb28 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;
@@ -4355,17 +4352,6 @@ static struct config_string ConfigureNamesString[] =
NULL, NULL, NULL
},
- {
- {"stats_temp_directory", PGC_SIGHUP, STATS_ACTIVITY,
- gettext_noop("Writes temporary statistics files to the specified directory."),
- NULL,
- GUC_SUPERUSER_ONLY
- },
- &pgstat_temp_directory,
- PG_STAT_TMP_DIR,
- check_canonical_path, assign_pgstat_temp_directory, NULL
- },
-
{
{"synchronous_standby_names", PGC_SIGHUP, REPLICATION_PRIMARY,
gettext_noop("Number of synchronous standbys and list of names of potential synchronous ones."),
@@ -11773,35 +11759,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 4f5b6bdb12..20c24a9d78 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -587,7 +587,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 c854221a30..0f42e78d19 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -217,7 +217,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 0472b728bf..d7c50eb4f9 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_Jan__8_10_24_34_2021_185)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v45-0007-Exclude-pg_stat-directory-from-base-backup.patch"
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Time delayed LR (WAS Re: logical replication restrictions)
@ 2023-01-24 02:45 Kyotaro Horiguchi <[email protected]>
0 siblings, 2 replies; 6+ messages in thread
From: Kyotaro Horiguchi @ 2023-01-24 02:45 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
> Attached the updated patch v19.
+ maybe_delay_apply(TransactionId xid, TimestampTz finish_ts)
I look this spelling strange. How about maybe_apply_delay()?
send_feedback():
+ * If the subscriber side apply is delayed (because of time-delayed
+ * replication) then do not tell the publisher that the received latest
+ * LSN is already applied and flushed, otherwise, it leads to the
+ * publisher side making a wrong assumption of logical replication
+ * progress. Instead, we just send a feedback message to avoid a publisher
+ * timeout during the delay.
*/
- if (!have_pending_txes)
+ if (!have_pending_txes && !in_delayed_apply)
flushpos = writepos = recvpos;
Honestly I don't like this wart. The reason for this is the function
assumes recvpos = applypos but we actually call it while holding
unapplied changes, that is, applypos < recvpos.
Couldn't we maintain an additional static variable "last_applied"
along with last_received? In this case the condition cited above
would be as follows and in_delayed_apply will become unnecessary.
+ if (!have_pending_txes && last_received == last_applied)
The function is a static function and always called with a variable
last_received that has the same scope with the function, as the first
parameter. Thus we can remove the first parameter then let the
function directly look at the both two varaibles instead.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Time delayed LR (WAS Re: logical replication restrictions)
@ 2023-01-24 03:05 Kyotaro Horiguchi <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
1 sibling, 0 replies; 6+ messages in thread
From: Kyotaro Horiguchi @ 2023-01-24 03:05 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
Sorry, I forgot to write one comment.
At Tue, 24 Jan 2023 11:45:35 +0900 (JST), Kyotaro Horiguchi <[email protected]> wrote in
+ /* Should we delay the current transaction? */
+ if (finish_ts)
+ maybe_delay_apply(xid, finish_ts);
+
if (!am_parallel_apply_worker())
maybe_start_skipping_changes(lsn);
It may not give actual advantages, but isn't it better that delay
happens after skipping?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Time delayed LR (WAS Re: logical replication restrictions)
@ 2023-01-24 06:57 Amit Kapila <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
1 sibling, 2 replies; 6+ messages in thread
From: Amit Kapila @ 2023-01-24 06:57 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
On Tue, Jan 24, 2023 at 8:15 AM Kyotaro Horiguchi
<[email protected]> wrote:
>
> > Attached the updated patch v19.
>
> + maybe_delay_apply(TransactionId xid, TimestampTz finish_ts)
>
> I look this spelling strange. How about maybe_apply_delay()?
>
+1.
>
> send_feedback():
> + * If the subscriber side apply is delayed (because of time-delayed
> + * replication) then do not tell the publisher that the received latest
> + * LSN is already applied and flushed, otherwise, it leads to the
> + * publisher side making a wrong assumption of logical replication
> + * progress. Instead, we just send a feedback message to avoid a publisher
> + * timeout during the delay.
> */
> - if (!have_pending_txes)
> + if (!have_pending_txes && !in_delayed_apply)
> flushpos = writepos = recvpos;
>
> Honestly I don't like this wart. The reason for this is the function
> assumes recvpos = applypos but we actually call it while holding
> unapplied changes, that is, applypos < recvpos.
>
> Couldn't we maintain an additional static variable "last_applied"
> along with last_received?
>
It won't be easy to maintain the meaning of last_applied because there
are cases where we don't apply the change directly. For example, in
case of streaming xacts, we will just keep writing it to the file,
now, say, due to some reason, we have to send the feedback, then it
will not allow you to update the latest write locations. This would
then become different then what we are doing without the patch.
Another point to think about is that we also need to keep the variable
updated for keep-alive ('k') messages even though we don't apply
anything in that case. Still, other cases to consider are where we
have mix of streaming and non-streaming transactions.
> In this case the condition cited above
> would be as follows and in_delayed_apply will become unnecessary.
>
> + if (!have_pending_txes && last_received == last_applied)
>
> The function is a static function and always called with a variable
> last_received that has the same scope with the function, as the first
> parameter. Thus we can remove the first parameter then let the
> function directly look at the both two varaibles instead.
>
I think this is true without this patch, so why that has not been
followed in the first place? One comment, I see in this regard is as
below:
/* It's legal to not pass a recvpos */
if (recvpos < last_recvpos)
recvpos = last_recvpos;
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 6+ messages in thread
* RE: Time delayed LR (WAS Re: logical replication restrictions)
@ 2023-01-24 12:32 Takamichi Osumi (Fujitsu) <[email protected]>
parent: Amit Kapila <[email protected]>
1 sibling, 0 replies; 6+ messages in thread
From: Takamichi Osumi (Fujitsu) @ 2023-01-24 12:32 UTC (permalink / raw)
To: 'Amit Kapila' <[email protected]>; Kyotaro Horiguchi <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; pgsql-hackers
On Tuesday, January 24, 2023 3:58 PM Amit Kapila <[email protected]> wrote:
> > send_feedback():
> > + * If the subscriber side apply is delayed (because of time-delayed
> > + * replication) then do not tell the publisher that the received latest
> > + * LSN is already applied and flushed, otherwise, it leads to the
> > + * publisher side making a wrong assumption of logical replication
> > + * progress. Instead, we just send a feedback message to avoid a
> publisher
> > + * timeout during the delay.
> > */
> > - if (!have_pending_txes)
> > + if (!have_pending_txes && !in_delayed_apply)
> > flushpos = writepos = recvpos;
> >
> > Honestly I don't like this wart. The reason for this is the function
> > assumes recvpos = applypos but we actually call it while holding
> > unapplied changes, that is, applypos < recvpos.
> >
> > Couldn't we maintain an additional static variable "last_applied"
> > along with last_received?
> >
>
> It won't be easy to maintain the meaning of last_applied because there are
> cases where we don't apply the change directly. For example, in case of
> streaming xacts, we will just keep writing it to the file, now, say, due to some
> reason, we have to send the feedback, then it will not allow you to update the
> latest write locations. This would then become different then what we are
> doing without the patch.
> Another point to think about is that we also need to keep the variable updated
> for keep-alive ('k') messages even though we don't apply anything in that case.
> Still, other cases to consider are where we have mix of streaming and
> non-streaming transactions.
Agreed. This will change some existing behaviors. So, didn't conduct this change in the latest patch [1].
[1] - https://www.postgresql.org/message-id/TYCPR01MB8373DC1881F382B4703F26E0EDC99%40TYCPR01MB8373.jpnprd0...
Best Regards,
Takamichi Osumi
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Time delayed LR (WAS Re: logical replication restrictions)
@ 2023-01-25 01:17 Kyotaro Horiguchi <[email protected]>
parent: Amit Kapila <[email protected]>
1 sibling, 0 replies; 6+ messages in thread
From: Kyotaro Horiguchi @ 2023-01-25 01:17 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers
In short, I'd like to propose renaming the parameter in_delayed_apply
of send_feedback to "has_unprocessed_change".
At Tue, 24 Jan 2023 12:27:58 +0530, Amit Kapila <[email protected]> wrote in
> > send_feedback():
> > + * If the subscriber side apply is delayed (because of time-delayed
> > + * replication) then do not tell the publisher that the received latest
> > + * LSN is already applied and flushed, otherwise, it leads to the
> > + * publisher side making a wrong assumption of logical replication
> > + * progress. Instead, we just send a feedback message to avoid a publisher
> > + * timeout during the delay.
> > */
> > - if (!have_pending_txes)
> > + if (!have_pending_txes && !in_delayed_apply)
> > flushpos = writepos = recvpos;
> >
> > Honestly I don't like this wart. The reason for this is the function
> > assumes recvpos = applypos but we actually call it while holding
> > unapplied changes, that is, applypos < recvpos.
> >
> > Couldn't we maintain an additional static variable "last_applied"
> > along with last_received?
> >
>
> It won't be easy to maintain the meaning of last_applied because there
> are cases where we don't apply the change directly. For example, in
> case of streaming xacts, we will just keep writing it to the file,
> now, say, due to some reason, we have to send the feedback, then it
> will not allow you to update the latest write locations. This would
> then become different then what we are doing without the patch.
> Another point to think about is that we also need to keep the variable
> updated for keep-alive ('k') messages even though we don't apply
> anything in that case. Still, other cases to consider are where we
> have mix of streaming and non-streaming transactions.
Yeah. Even though I named it as "last_applied", its objective is to
have get_flush_position returning the correct have_pending_txes
without a hint from callers, that is, "let g_f_position know if
store_flush_position has been called with the last received data".
Anyway I tried that but didn't find a clean and simple way. However,
while on it, I realized what the code made me confused.
+static void send_feedback(XLogRecPtr recvpos, bool force, bool requestReply,
+ bool in_delayed_apply);
The name "in_delayed_apply" doesn't donsn't give me an idea of what
the function should do for it. If it is named "has_unprocessed_change",
I think it makes sense that send_feedback should think there may be an
outstanding transaction that is not known to the function.
So, my conclusion here is I'd like to propose changing the parameter
name to "has_unapplied_change".
> > In this case the condition cited above
> > would be as follows and in_delayed_apply will become unnecessary.
> >
> > + if (!have_pending_txes && last_received == last_applied)
> >
> > The function is a static function and always called with a variable
> > last_received that has the same scope with the function, as the first
Sorry for the noise, I misread it. Maybe I took the "function-scoped"
variable as file-scoped.. Thus the discussion is false.
> > parameter. Thus we can remove the first parameter then let the
> > function directly look at the both two varaibles instead.
> >
>
> I think this is true without this patch, so why that has not been
> followed in the first place? One comment, I see in this regard is as
> below:
>
> /* It's legal to not pass a recvpos */
> if (recvpos < last_recvpos)
> recvpos = last_recvpos;
Sorry. I don't understand this. It is just a part of the ratchet
mechanism for the last received lsn to report.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2023-01-25 01:17 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-09-29 13:59 [PATCH v45 6/7] Remove the GUC stats_temp_directory Kyotaro Horiguchi <[email protected]>
2023-01-24 02:45 Re: Time delayed LR (WAS Re: logical replication restrictions) Kyotaro Horiguchi <[email protected]>
2023-01-24 03:05 ` Re: Time delayed LR (WAS Re: logical replication restrictions) Kyotaro Horiguchi <[email protected]>
2023-01-24 06:57 ` Re: Time delayed LR (WAS Re: logical replication restrictions) Amit Kapila <[email protected]>
2023-01-24 12:32 ` RE: Time delayed LR (WAS Re: logical replication restrictions) Takamichi Osumi (Fujitsu) <[email protected]>
2023-01-25 01:17 ` Re: Time delayed LR (WAS Re: logical replication restrictions) Kyotaro Horiguchi <[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