Re: tweak to a few index tests to hits ambuildempty() routine.
On Wed, Sep 14, 2022 at 12:16 PM wrote: > > I still wonder, if assert doesn't catch why that place is marked as > covered here? > https://coverage.postgresql.org/src/backend/access/gin/ginvacuum.c.gcov.html > Probably other tests cover that. Regards, Amul
Query jumbling for prepare statement
Hi! I found prepare statement are not jumbled. Fro example PREPARE 't1'; and PREPARE 't2' are counted separately in pg_stat_statements. I think it needs to be fixed. What do you think? Regards, Kotaro Kawamoto
Re: [PATCH]Feature improvement for MERGE tab completion
On 2022/09/14 14:08, bt22kawamotok wrote:
When I tried to apply this patch, I got the following warning, please fix it.
Other than that, I think everything is fine.
$ git apply fix_tab_completion_merge_v4.patch
fix_tab_completion_merge_v4.patch:38: trailing whitespace.
else if (TailMatches("USING", MatchAny, "ON", MatchAny) ||
fix_tab_completion_merge_v4.patch:39: indent with spaces.
TailMatches("USING", MatchAny, "AS", MatchAny, "ON",
MatchAny) ||
fix_tab_completion_merge_v4.patch:40: indent with spaces.
TailMatches("USING", MatchAny, MatchAny, "ON", MatchAny))
fix_tab_completion_merge_v4.patch:53: trailing whitespace.
else if (TailMatches("WHEN", "MATCHED") ||
warning: 4 lines add whitespace errors.
Thanks for reviewing.
I fixed the problem and make patch v5.
Please check it.
Thanks for updating the patch!
+ else if (TailMatches("MERGE", "INTO", MatchAny, "USING") ||
+TailMatches("MERGE", "INTO", MatchAny, MatchAny,
"USING") ||
+TailMatches("MERGE", "INTO", MatchAny, "AS", MatchAny,
"USING"))
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables);
+ else if (TailMatches("MERGE", "INTO", MatchAny, "AS", MatchAny,
"USING") ||
+TailMatches("MERGE", "INTO", MatchAny, MatchAny,
"USING"))
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables);
The latter seems redundant and can be removed. The former seems to
cover all the cases where the latter covers.
Not only table but also view, foreign table, etc can be specified after
USING in MERGE command. So ISTM that Query_for_list_of_selectables
should be used at the above tab-completion, instead of Query_for_list_of_tables.
Thought?
+ else if (TailMatches("USING", MatchAny, "ON", MatchAny) ||
+TailMatches("USING", MatchAny, "ON", MatchAny,
MatchAnyExcept("When"), MatchAnyExcept("When")) ||
+TailMatches("USING", MatchAny, "AS", MatchAny, "ON",
MatchAny) ||
+TailMatches("USING", MatchAny, "AS", MatchAny, "ON", MatchAny,
MatchAnyExcept("When"), MatchAnyExcept("When")) ||
+TailMatches("USING", MatchAny, MatchAny, "ON",
MatchAny) ||
+TailMatches("USING", MatchAny, MatchAny, "ON", MatchAny,
MatchAnyExcept("When"), MatchAnyExcept("When")))
"When" should be "WHEN"?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: Query jumbling for prepare statement
Hi, On Wed, Sep 14, 2022 at 05:14:06PM +0900, bt22kawamotok wrote: > > I found prepare statement are not jumbled. > Fro example PREPARE 't1'; and PREPARE 't2' are counted separately in > pg_stat_statements. Are you talking about PREPARE TRANSACTION? If yes I already suggested that a few days ago (1), and Bertrand implemented it in the v5 version of the patch on that thread. [1] https://www.postgresql.org/message-id/20220908112919.2ytxpkitiw6lt2u6@jrouhaud
Re: pg_basebackup's --gzip switch misbehaves
> On 13 Sep 2022, at 23:38, Tom Lane wrote: > The $tempdir is some temporary subdirectory of tmp_check that will get nuked > at > the end of the TAP test no matter what. So these rmtrees are merely making > the > evidence disappear a bit faster; it will anyway. Maybe the creation of $tempdir should take PG_TEST_NOCLEAN into account and not register CLEANUP if set? -- Daniel Gustafsson https://vmware.com/
Re: Query jumbling for prepare statement
2022-09-14 17:18 に Julien Rouhaud さんは書きました: Hi, On Wed, Sep 14, 2022 at 05:14:06PM +0900, bt22kawamotok wrote: I found prepare statement are not jumbled. Fro example PREPARE 't1'; and PREPARE 't2' are counted separately in pg_stat_statements. Are you talking about PREPARE TRANSACTION? If yes I already suggested that a few days ago (1), and Bertrand implemented it in the v5 version of the patch on that thread. [1] https://www.postgresql.org/message-id/20220908112919.2ytxpkitiw6lt2u6@jrouhaud Oh, I had missed it. Thanks for telling me about it. Kotaro Kawmaoto
Re: Refactor backup related code (was: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?)
On Mon, Sep 12, 2022 at 5:09 PM Bharath Rupireddy
wrote:
>
> Please review the attached v3 patch after removing the above macro changes.
I'm attaching the v4 patch that's rebased on to the latest HEAD.
Please consider this for review.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From a6f39ab4391d2a7582f354888c68b908e92653d8 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy
Date: Wed, 14 Sep 2022 08:50:10 +
Subject: [PATCH v4] Refactor backup related code
This patch tries to refactor backup related code, advantages of
doing so are following:
1) backup state is more structured now - all in a single structure,
callers can create backup_label contents whenever required, either
during the pg_backup_start or the pg_backup_stop or in between.
2) no parsing of backup_label file lines now, no error checking
for invalid parsing.
3) backup_label and history file contents have most of the things
in common, they can now be created within a single function
4) makes backup related code extensible and readable
---
src/backend/access/transam/xlog.c | 318 +
src/backend/access/transam/xlogfuncs.c | 42 ++--
src/backend/backup/basebackup.c| 31 ++-
src/include/access/xlog.h | 12 +-
src/include/access/xlog_internal.h | 25 ++
5 files changed, 231 insertions(+), 197 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 81d339d57d..d8361272a4 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8233,62 +8233,137 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
PendingWalStats.wal_sync++;
}
+/*
+ * Get backup state.
+ */
+BackupState
+get_backup_state(const char *name)
+{
+ BackupState state;
+ Size len;
+
+ state = (BackupState) palloc0(sizeof(BackupStateData));
+ len = strlen(name);
+ state->name = (char *) palloc0(len + 1);
+ memcpy(state->name, name, len);
+ state->backup_label = makeStringInfo();
+ state->tablespace_map = makeStringInfo();
+ state->history_file = makeStringInfo();
+
+ return state;
+}
+
+/*
+ * Free backup state.
+ */
+void
+free_backup_state(BackupState state)
+{
+ Assert(state != NULL);
+
+ pfree(state->name);
+ pfree(state->backup_label->data);
+ pfree(state->tablespace_map->data);
+ pfree(state->history_file->data);
+ pfree(state);
+}
+
+/*
+ * Construct backup_label or history file content strings.
+ */
+void
+create_backup_content_str(BackupState state, bool forhistoryfile)
+{
+ StringInfo str;
+ char startstrbuf[128];
+ char stopstrfbuf[128];
+
+ if (forhistoryfile)
+ str = state->history_file;
+ else
+ str = state->backup_label;
+
+ /* Use the log timezone here, not the session timezone */
+ pg_strftime(startstrbuf, sizeof(startstrbuf), "%Y-%m-%d %H:%M:%S %Z",
+pg_localtime(&state->starttime, log_timezone));
+
+ appendStringInfo(str, "START WAL LOCATION: %X/%X (file %s)\n",
+ LSN_FORMAT_ARGS(state->startpoint),
+ state->startxlogfile);
+
+ if (forhistoryfile)
+ appendStringInfo(str, "STOP WAL LOCATION: %X/%X (file %s)\n",
+ LSN_FORMAT_ARGS(state->startpoint),
+ state->stopxlogfile);
+
+ appendStringInfo(str, "CHECKPOINT LOCATION: %X/%X\n",
+ LSN_FORMAT_ARGS(state->checkpointloc));
+ appendStringInfo(str, "BACKUP METHOD: streamed\n");
+ appendStringInfo(str, "BACKUP FROM: %s\n",
+ state->started_in_recovery ? "standby" : "primary");
+ appendStringInfo(str, "START TIME: %s\n", startstrbuf);
+ appendStringInfo(str, "LABEL: %s\n", state->name);
+ appendStringInfo(str, "START TIMELINE: %u\n", state->starttli);
+
+ if (forhistoryfile)
+ {
+ /* Use the log timezone here, not the session timezone */
+ pg_strftime(stopstrfbuf, sizeof(stopstrfbuf), "%Y-%m-%d %H:%M:%S %Z",
+ pg_localtime(&state->stoptime, log_timezone));
+
+ appendStringInfo(str, "STOP TIME: %s\n", stopstrfbuf);
+ appendStringInfo(str, "STOP TIMELINE: %u\n", state->stoptli);
+ }
+}
+
/*
* do_pg_backup_start is the workhorse of the user-visible pg_backup_start()
* function. It creates the necessary starting checkpoint and constructs the
- * backup label and tablespace map.
- *
- * Input parameters are "backupidstr" (the backup label string) and "fast"
- * (if true, we do the checkpoint in immediate mode to make it faster).
+ * backup state.
*
- * The backup label and tablespace map contents are appended to *labelfile and
- * *tblspcmapfile, and the caller is responsible for including them in the
- * backup archive as 'backup_label' and 'tablespace_map'.
- * tblspcmapfile is required mainly for tar format in windows as native windows
- * utilities are not able to create symlinks while extracting files from tar.
- * However for consistency and platform-independence, we do it the same way
- * everywhere.
+ * Input parameters are "state" (containing backup state), "fast" (if true,
+ * we do the checkpoint in immediate mode to make it faster) and "table
Re: [PATCH]Feature improvement for MERGE tab completion
+ else if (TailMatches("MERGE", "INTO", MatchAny, "USING") ||
+TailMatches("MERGE", "INTO", MatchAny, MatchAny,
"USING") ||
+TailMatches("MERGE", "INTO", MatchAny, "AS", MatchAny,
"USING"))
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables);
+ else if (TailMatches("MERGE", "INTO", MatchAny, "AS", MatchAny,
"USING") ||
+TailMatches("MERGE", "INTO", MatchAny, MatchAny,
"USING"))
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables);
The latter seems redundant and can be removed. The former seems to
cover all the cases where the latter covers.
+ else if (TailMatches("USING", MatchAny, "ON", MatchAny) ||
+TailMatches("USING", MatchAny, "ON", MatchAny,
MatchAnyExcept("When"), MatchAnyExcept("When")) ||
+TailMatches("USING", MatchAny, "AS", MatchAny, "ON",
MatchAny) ||
+TailMatches("USING", MatchAny, "AS", MatchAny, "ON",
MatchAny,
MatchAnyExcept("When"), MatchAnyExcept("When")) ||
+TailMatches("USING", MatchAny, MatchAny, "ON",
MatchAny) ||
+TailMatches("USING", MatchAny, MatchAny, "ON",
MatchAny,
MatchAnyExcept("When"), MatchAnyExcept("When")))
"When" should be "WHEN"?
Regards,
Thanks for reviewing.
Sorry for making such a simple mistake.
I fixed it in v6.
Not only table but also view, foreign table, etc can be specified after
USING in MERGE command. So ISTM that Query_for_list_of_selectables
should be used at the above tab-completion, instead of
Query_for_list_of_tables.
Thought?
That's nice idea!
I took that in v6.
Regards,
Kotaro Kawamoto
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 62a39779b9..70af5acf12 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -4063,23 +4063,25 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_mergetargets);
else if (TailMatches("MERGE", "INTO", MatchAny))
COMPLETE_WITH("USING", "AS");
- else if (TailMatches("MERGE", "INTO", MatchAny, "USING"))
- COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables);
/* with [AS] alias */
- else if (TailMatches("MERGE", "INTO", MatchAny, "AS", MatchAny))
- COMPLETE_WITH("USING");
- else if (TailMatches("MERGE", "INTO", MatchAny, MatchAny))
+ else if (TailMatches("MERGE", "INTO", MatchAny, "AS", MatchAny) ||
+ TailMatches("MERGE", "INTO", MatchAny, MatchAnyExcept("AS")))
COMPLETE_WITH("USING");
- else if (TailMatches("MERGE", "INTO", MatchAny, "AS", MatchAny, "USING"))
- COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables);
- else if (TailMatches("MERGE", "INTO", MatchAny, MatchAny, "USING"))
- COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables);
+ else if (TailMatches("MERGE", "INTO", MatchAny, "USING") ||
+ TailMatches("MERGE", "INTO", MatchAny, "AS", MatchAny, "USING") ||
+ TailMatches("MERGE", "INTO", MatchAny, MatchAny, "USING"))
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_selectables);
+ else if (TailMatches("MERGE", "INTO", MatchAny, "USING", MatchAny) ||
+ TailMatches("MERGE", "INTO", MatchAny, MatchAny, "USING", MatchAny) ||
+ TailMatches("MERGE", "INTO", MatchAny, "AS", MatchAny, "USING", MatchAny))
+ COMPLETE_WITH("AS", "ON");
/* ON */
- else if (TailMatches("MERGE", "INTO", MatchAny, "USING", MatchAny))
- COMPLETE_WITH("ON");
- else if (TailMatches("INTO", MatchAny, "AS", MatchAny, "USING", MatchAny, "AS", MatchAny))
- COMPLETE_WITH("ON");
- else if (TailMatches("INTO", MatchAny, MatchAny, "USING", MatchAny, MatchAny))
+ else if (TailMatches("MERGE", "INTO", MatchAny, "USING", MatchAny, MatchAnyExcept("ON|AS")) ||
+ TailMatches("MERGE", "INTO", MatchAny, "USING", MatchAny, "AS", MatchAny) ||
+ TailMatches("MERGE", "INTO", MatchAny, MatchAny, "USING", MatchAny, MatchAnyExcept("ON|AS")) ||
+ TailMatches("MERGE", "INTO", MatchAny, MatchAny, "USING", MatchAny, "AS", MatchAny) ||
+ TailMatches("MERGE", "INTO", MatchAny, "AS", MatchAny, "USING", MatchAny, MatchAnyExcept("ON|AS")) ||
+ TailMatches("MERGE", "INTO", MatchAny, "AS", MatchAny, "USING", MatchAny, "AS", MatchAny))
COMPLETE_WITH("ON");
/* ON condition */
else if (TailMatches("INTO", MatchAny, "USING", MatchAny, "ON"))
@@ -4089,18 +4091,25 @@ psql_completion(const char *text, int start, int end)
else if (TailMatches("INTO", MatchAny, MatchAny, "USING", MatchAny, MatchAny, "ON"))
COMPLETE_WITH_ATTR(prev6_wd);
/* WHEN [NOT] MATCHED */
- else if (TailMatches("USING", MatchAny, "ON", MatchAny))
- COMPLETE_WITH("WHEN MATCHED", "WHEN NOT MATCHED");
- else if (TailMatches("USING", MatchAny, "AS", MatchAny, "ON", MatchAny))
+ else if (TailMatches("USING", MatchAny, "ON", MatchAny) ||
+ TailMatches("USING", MatchAny, "ON", MatchAny, MatchAnyExcept("WHEN"), MatchAnyExcept("WHEN")) ||
+ TailMatches("USING", MatchAny, "AS", MatchAny, "ON", MatchAny
FTS parser - missing UUID token type
I miss UUID, which indexes very strangely, is more and more popular and people want to search for it. See: https://www.postgresql.org/docs/current/textsearch-parsers.html UUID is fairly easy to parse: The hexadecimal digits are grouped as 32 hexadecimal characters with four hyphens: ----. The number of characters per hyphen is 8-4-4-4-12. The last section of four, or the N position, indicates the format and encoding in either one to three bits. Now, UUIDs parse each other differently, depending on whether the individual parts begin with numbers or letters: 00633f1d-1fff-409e-8294-40a21f565904 '-40':6 '00633f1d':2 '00633f1d-1fff-409e':1 '1fff':3 '409e':4 '8294':5 'a21f565904':7 00856c28-2251-4aaf-82d3-e4962f5b732d '-2251':2 '-4':3 '00856c28':1 '82d3':6 'aaf':5 'aaf-82d3-e4962f5b732d':4 'e4962f5b732d':7 00a1cc84-816a-490a-a99c-8a4c637380b0 '00a1cc84':2 '00a1cc84-816a-490a-a99c-8a4c637380b0':1 '490a':4 '816a':3 '8a4c637380b0':6 'a99c':5 As a result, such identifiers cannot be found in the database later. What is your opinion on missing tokens for FTS? -- Przemysław Sztoch | Mobile +48 509 99 00 66
Re: Improve description of XLOG_RUNNING_XACTS
On Fri, Sep 9, 2022 at 6:18 AM Masahiko Sawada wrote: > > Updated the patch accordingly. > I have created two xacts each with savepoints and after your patch, the record will show xacts/subxacts information as below: rmgr: Standby len (rec/tot): 74/74, tx: 0, lsn: 0/014AC238, prev 0/014AC1F8, desc: RUNNING_XACTS nextXid 733 latestCompletedXid 726 oldestRunningXid 727; 2 xacts: 729 727; 4 subxacts: 730 731 728 732 There is no way to associate which subxacts belong to which xact, so will it be useful, and if so, how? I guess we probably don't need it here because the describe records just display the record information. -- With Regards, Amit Kapila.
Re: pgsql: Doc: Explain about Column List feature.
On 2022-Sep-14, Peter Smith wrote: > PSA a new patch for the "Column Lists" page. AFAIK this is the same as > everything that you suggested I don't get it. You send me my patch back, and claim it is a new patch? I kindly request that when you review a patch, you do not hijack the submitter's patch and claim it as your own. If a submitter goes mising or states that they're unavailable to complete some work, then that's okay, but otherwise it seems a bit offensive to me. I have seen that repeatedly of late, and I find it quite rude. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Las cosas son buenas o malas segun las hace nuestra opinión" (Lisias)
Inconsistencies in error messages
Hi,
When translating error messages, Alexander Lakhin
() noticed some inconsistencies so I prepared a
small patch to fix those.
Please see attached.
--
Ekaterina Kiryanova
Technical Writer
Postgres Professional
the Russian PostgreSQL Companydiff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 1024d51dca..27cc5f0e65 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1187,7 +1187,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("ALTER SUBSCRIPTION with refresh and copy_data is not allowed when two_phase is enabled"),
- errhint("Use ALTER SUBSCRIPTION ...SET PUBLICATION with refresh = false, or with copy_data = false"
+ errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION with refresh = false, or with copy_data = false"
", or use DROP/CREATE SUBSCRIPTION.")));
PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with refresh");
@@ -1239,7 +1239,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("ALTER SUBSCRIPTION with refresh and copy_data is not allowed when two_phase is enabled"),
- errhint("Use ALTER SUBSCRIPTION ...SET PUBLICATION with refresh = false, or with copy_data = false"
+ errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION with refresh = false, or with copy_data = false"
", or use DROP/CREATE SUBSCRIPTION.")));
PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with refresh");
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 40601aefd9..8dd7d64630 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -663,7 +663,7 @@ SanityCheckBackgroundWorker(BackgroundWorker *worker, int elevel)
{
ereport(elevel,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("background worker \"%s\": background worker without shared memory access are not supported",
+ errmsg("background worker \"%s\": background workers without shared memory access are not supported",
worker->bgw_name)));
return false;
}
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 29ae27e5e3..d02fd83c0a 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -578,7 +578,7 @@ rewriteRuleAction(Query *parsetree,
if (sub_action->hasModifyingCTE && rule_action != sub_action)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("INSERT...SELECT rule actions are not supported for queries having data-modifying statements in WITH")));
+ errmsg("INSERT ... SELECT rule actions are not supported for queries having data-modifying statements in WITH")));
}
/*
diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out
index 30dd900e11..7f2e32d8b0 100644
--- a/src/test/regress/expected/with.out
+++ b/src/test/regress/expected/with.out
@@ -2515,14 +2515,14 @@ SELECT * FROM bug6051_2;
3
(3 rows)
--- check INSERT...SELECT rule actions are disallowed on commands
+-- check INSERT ... SELECT rule actions are disallowed on commands
-- that have modifyingCTEs
CREATE OR REPLACE RULE bug6051_ins AS ON INSERT TO bug6051 DO INSTEAD
INSERT INTO bug6051_2
SELECT NEW.i;
WITH t1 AS ( DELETE FROM bug6051 RETURNING * )
INSERT INTO bug6051 SELECT * FROM t1;
-ERROR: INSERT...SELECT rule actions are not supported for queries having data-modifying statements in WITH
+ERROR: INSERT ... SELECT rule actions are not supported for queries having data-modifying statements in WITH
-- silly example to verify that hasModifyingCTE flag is propagated
CREATE TEMP TABLE bug6051_3 AS
SELECT a FROM generate_series(11,13) AS a;
diff --git a/src/test/regress/sql/with.sql b/src/test/regress/sql/with.sql
index 5c52561a8a..0f5730797f 100644
--- a/src/test/regress/sql/with.sql
+++ b/src/test/regress/sql/with.sql
@@ -1169,7 +1169,7 @@ INSERT INTO bug6051 SELECT * FROM t1;
SELECT * FROM bug6051;
SELECT * FROM bug6051_2;
--- check INSERT...SELECT rule actions are disallowed on commands
+-- check INSERT ... SELECT rule actions are disallowed on commands
-- that have modifyingCTEs
CREATE OR REPLACE RULE bug6051_ins AS ON INSERT TO bug6051 DO INSTEAD
INSERT INTO bug6051_2
Re: New strategies for freezing, advancing relfrozenxid early
On Wed, Sep 14, 2022 at 12:53 AM Peter Geoghegan wrote: > This is still only scratching the surface of what is possible with > dead_items. The visibility map snapshot concept can enable a far more > sophisticated approach to resource management in vacuumlazy.c. It > could help us to replace a simple array of item pointers (the current > dead_items array) with a faster and more space-efficient data > structure. Masahiko Sawada has done a lot of work on this recently, so > this may interest him. I don't quite see how it helps "enable" that. It'd be more logical to me to say the VM snapshot *requires* you to think harder about resource management, since a palloc'd snapshot should surely be counted as part of the configured memory cap that admins control. (Commonly, it'll be less than a few dozen MB, so I'll leave that aside.) Since Masahiko hasn't (to my knowlege) gone as far as integrating his ideas into vacuum, I'm not sure if the current state of affairs has some snag that a snapshot will ease, but if there is, you haven't described what it is. I do remember your foreshadowing in the radix tree thread a while back, and I do think it's an intriguing idea to combine pages-to-scan and dead TIDs in the same data structure. The devil is in the details, of course. It's worth looking into. > VM snapshots could also make it practical for the new data structure > to spill to disk to avoid multiple index scans/passed by VACUUM. I'm not sure spilling to disk is solving the right problem (as opposed to the hash join case, or to the proposed conveyor belt system which has a broader aim). I've found several times that a customer will ask if raising maintenance work mem from 1GB to 10GB will make vacuum faster. Looking at the count of index scans, it's pretty much always "1", so even if the current approach could scale above 1GB, "no" it wouldn't help to raise that limit. Your mileage may vary, of course. Continuing my customer example, searching the dead TID list faster *will* make vacuum faster. The proposed tree structure is more memory efficient, and IIUC could scale beyond 1GB automatically since each node is a separate allocation, so the answer will be "yes" in the rare case the current setting is in fact causing multiple index scans. Furthermore, it doesn't have to anticipate the maximum size, so there is no up front calculation assuming max-tuples-per-page, so it automatically uses less memory for less demanding tables. (But +1 for changing that calculation for as long as we do have the single array.) -- John Naylor EDB: http://www.enterprisedb.com
Re: Inconsistencies in error messages
On Wed, Sep 14, 2022 at 5:01 PM Ekaterina Kiryanova
wrote:
>
> Hi,
>
> When translating error messages, Alexander Lakhin
> () noticed some inconsistencies so I prepared a
> small patch to fix those.
+1
This one
- errmsg("background worker \"%s\": background worker without shared
memory access are not supported",
+ errmsg("background worker \"%s\": background workers without shared
memory access are not supported",
is a grammar error so worth backpatching, but the rest are cosmetic.
Will commit this way unless there are objections.
--
John Naylor
EDB: http://www.enterprisedb.com
Re: Inconsistencies in error messages
On 2022-Sep-14, John Naylor wrote:
> This one
>
> + errmsg("background worker \"%s\": background workers without shared
> memory access are not supported",
>
> is a grammar error so worth backpatching, but the rest are cosmetic.
>
> Will commit this way unless there are objections.
+1
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"I dream about dreams about dreams", sang the nightingale
under the pale moon (Sandman)
Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher
Hi, > > > > Oh, I haven't considered inherited tables. That seems right, the > statistics of the children are not updated when the parent is analyzed. > > > >> > >> Now, the point I was worried about was what if the changes in child > >> tables (*_part1, *_part2) are much more than in tbl1? In such cases, > >> we may not invalidate child rel entries, so how will logical > >> replication behave for updates/deletes on child tables? There may not > >> be any problem here but it is better to do some analysis of such cases > >> to see how it behaves. > > > > > > I also haven't observed any specific issues. In the end, when the user > (or autovacuum) does ANALYZE on the child, it is when the statistics are > updated for the child. > > > > Right, I also think that should be the behavior but I have not > verified it. However, I think it should be easy to verify if > autovacuum updates the stats for child tables when we operate on only > one of such tables and whether that will invalidate the cache for our > case. > > I already added a regression test for this with the title: # Testcase start: SUBSCRIPTION CAN UPDATE THE INDEX IT USES AFTER ANALYZE - INHERITED TABLE I realized that the comments on the test case were confusing, and clarified those. Attached the new version also rebased onto the master branch. Thanks, Onder v10_0001_use_index_on_subs_when_pub_rep_ident_full.patch Description: Binary data
RE: logical replication restrictions
Hi,
Sorry for noise but I found another bug.
When the 032_apply_delay.pl is modified like following,
the test will be always failed even if my patch is applied.
```
# Disable subscription. worker should die immediately.
-$node_subscriber->safe_psql('postgres',
- "ALTER SUBSCRIPTION tap_sub DISABLE"
+$node_subscriber->safe_psql('postgres', q{
+BEGIN;
+ALTER SUBSCRIPTION tap_sub DISABLE;
+SELECT pg_sleep(1);
+COMMIT;
+}
);
```
The point of failure is same as I reported previously.
```
...
2022-09-14 12:00:48.891 UTC [11330] 032_apply_delay.pl LOG: statement: ALTER
SUBSCRIPTION tap_sub SET (min_apply_delay = 8646)
2022-09-14 12:00:48.910 UTC [11226] DEBUG: sending feedback (force 0) to recv
0/1690220, write 0/1690220, flush 0/1690220
2022-09-14 12:00:48.937 UTC [11208] DEBUG: server process (PID 11328) exited
with exit code 0
2022-09-14 12:00:48.950 UTC [11226] DEBUG: logical replication apply delay:
86459996 ms
2022-09-14 12:00:48.950 UTC [11226] CONTEXT: processing remote data for
replication origin "pg_16393" during "BEGIN" in transaction 734 finished at
0/16902A8
2022-09-14 12:00:48.979 UTC [11208] DEBUG: forked new backend, pid=11334
socket=6
2022-09-14 12:00:49.007 UTC [11334] 032_apply_delay.pl LOG: statement: BEGIN;
2022-09-14 12:00:49.008 UTC [11334] 032_apply_delay.pl LOG: statement: ALTER
SUBSCRIPTION tap_sub DISABLE;
2022-09-14 12:00:49.009 UTC [11334] 032_apply_delay.pl LOG: statement: SELECT
pg_sleep(1);
2022-09-14 12:00:49.009 UTC [11226] DEBUG: check status of MySubscription
2022-09-14 12:00:49.009 UTC [11226] CONTEXT: processing remote data for
replication origin "pg_16393" during "BEGIN" in transaction 734 finished at
0/16902A8
2022-09-14 12:00:49.009 UTC [11226] DEBUG: logical replication apply delay:
86459937 ms
2022-09-14 12:00:49.009 UTC [11226] CONTEXT: processing remote data for
replication origin "pg_16393" during "BEGIN" in transaction 734 finished at
0/16902A8
...
```
I think it may be caused that waken worker read catalogs that have not modified
yet.
In AlterSubscription(), the backend kicks the apply worker ASAP, but it should
be at
end of the transaction, like ApplyLauncherWakeupAtCommit() and
AtEOXact_ApplyLauncher().
```
+ /*
+* If this subscription has been disabled and
it has an apply
+* delay set, wake up the logical replication
worker to finish
+* it as soon as possible.
+*/
+ if (!opts.enabled && sub->applydelay > 0)
+ logicalrep_worker_wakeup(sub->oid,
InvalidOid);
+
```
How do you think?
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Re: Avoid redudant initialization and possible memory leak (src/backend/parser/parse_relation.c)
On 2022-Sep-13, Ranier Vilela wrote: > Yeah, as per Julien's answer, there is really no memory leak, but just > unnecessary double execution of pstrdup. > But for Postgres 15, I believe it's worth avoiding this, because it's > wasted cycles. Yeah, this is a merge mistake. Fix applied, thanks. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: FTS parser - missing UUID token type
=?UTF-8?Q?Przemys=c5=82aw_Sztoch?= writes: > I miss UUID, which indexes very strangely, is more and more popular and > people want to search for it. Really? UUIDs in running text seem like an extremely uncommon use-case to me. URLs in running text are common nowadays, which is why the text search parser has special code for that, but UUIDs? Adding such a thing isn't cost-free either. Aside from the probably-substantial development effort, we know from experience with the URL support that it sometimes misfires and identifies something as a URL or URL fragment when it really isn't one. That leads to poorer indexing of the affected text. It seems likely that adding a UUID token type would be a net negative for most people, since they'd be subject to that hazard even if their text contains no true UUIDs. It's a shame that the text search parser isn't more extensible. If it were you could imagine having such a feature while making it optional. I'm not volunteering to fix that though :-( regards, tom lane
Re: ICU for global collation
Hello!
I was surprised that it is allowed to create clusters/databases where
the default ICU collations do not actually work due to unsupported
encodings:
$ initdb --encoding SQL_ASCII --locale-provider icu --icu-locale en-US
-D data &&
pg_ctl -D data -l logfile start &&
psql -c "SELECT 'a' < 'b'" template1
...
waiting for server to start done
server started
ERROR: encoding "SQL_ASCII" not supported by ICU
$ createdb --encoding SQL_ASCII --locale-provider icu --icu-locale en-US
--template template0 mydb &&
psql -c "SELECT 'a' < 'b'" mydb
ERROR: encoding "SQL_ASCII" not supported by ICU
The patch diff_check_icu_encoding.patch prohibits the creation of such
objects...
--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 6ff48bb18f3639ae45d9528b32df51a4aebc60c0..07758d15e8613d5a049537ddf2c5992e57ad6424 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -1042,11 +1042,16 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("ICU locale must be specified")));
- }
- if (dblocprovider == COLLPROVIDER_ICU)
check_icu_locale(dbiculocale);
+ if (!(is_encoding_supported_by_icu(encoding)))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("encoding \"%s\" is not supported with ICU provider",
+ pg_encoding_to_char(encoding;
+ }
+
/*
* Check that the new encoding and locale settings match the source
* database. We insist on this because we simply copy the source data ---
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index e00837ecacf4885cf2a176168c283f3e67c6eb53..8a762ced8340c9d8256f7832a4c19a43f1d5538a 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -2362,6 +2362,16 @@ setup_locale_encoding(void)
if (!check_locale_encoding(lc_ctype, encodingid) ||
!check_locale_encoding(lc_collate, encodingid))
exit(1);/* check_locale_encoding printed the error */
+
+ if (locale_provider == COLLPROVIDER_ICU &&
+ !(is_encoding_supported_by_icu(encodingid)))
+ {
+ pg_log_error("encoding \"%s\" is not supported with ICU provider",
+ pg_encoding_to_char(encodingid));
+ pg_log_error_hint("Rerun %s and choose a matching combination.",
+ progname);
+ exit(1);
+ }
}
diff --git a/src/bin/initdb/t/001_initdb.pl b/src/bin/initdb/t/001_initdb.pl
index a37f6dd9b334b6ee22d9fdd4d51422795cb54a39..e4bb3d0cdd9c23729c5fb97886374f8df558f239 100644
--- a/src/bin/initdb/t/001_initdb.pl
+++ b/src/bin/initdb/t/001_initdb.pl
@@ -118,6 +118,15 @@ if ($ENV{with_icu} eq 'yes')
],
qr/FATAL: could not open collator for locale/,
'fails for invalid ICU locale');
+
+ command_fails_like(
+ [
+ 'initdb','--no-sync',
+ '--locale-provider=icu', '--icu-locale=en',
+ '--encoding=SQL_ASCII', "$tempdir/dataX"
+ ],
+ qr/error: encoding "SQL_ASCII" is not supported with ICU provider/,
+ 'encoding "SQL_ASCII" is not supported with ICU provider');
}
else
{
diff --git a/src/bin/scripts/t/020_createdb.pl b/src/bin/scripts/t/020_createdb.pl
index e91c1d013d08d8bd1e3a92f2aba958c5c7713ca6..eaab3caa32669ead068719d98bb953c5c6ff5a17 100644
--- a/src/bin/scripts/t/020_createdb.pl
+++ b/src/bin/scripts/t/020_createdb.pl
@@ -50,6 +50,16 @@ if ($ENV{with_icu} eq 'yes')
],
'fails for invalid ICU locale');
+ $node->command_fails_like(
+ [
+ 'createdb','-T',
+ 'template0', '--locale-provider=icu',
+ '--icu-locale=en', '--encoding=SQL_ASCII',
+ 'foobarX'
+ ],
+ qr/ERROR: encoding "SQL_ASCII" is not supported with ICU provider/,
+ 'encoding "SQL_ASCII" is not supported with ICU provider');
+
# additional node, which uses the icu provider
my $node2 = PostgreSQL::Test::Cluster->new('icu');
$node2->init(extra => ['--locale-provider=icu', '--icu-locale=en']);
Re: SUBTRANS: Minimizing calls to SubTransSetParent()
On 2022-Aug-30, Simon Riggs wrote: > 001_new_isolation_tests_for_subxids.v3.patch > Adds new test cases to master without adding any new code, specifically > addressing the two areas of code that are not tested by existing tests. > This gives us a baseline from which we can do test driven development. > I'm hoping this can be reviewed and committed fairly smoothly. I gave this a quick run to confirm the claimed increase of coverage. It checks out, so pushed. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Re: pg_receivewal and SIGTERM
> On 2 Sep 2022, at 10:00, Michael Paquier wrote: > > On Fri, Aug 26, 2022 at 09:51:26AM +0900, Michael Paquier wrote: >> Fine by me if both you and Daniel want to be more careful with this >> change. We could always argue about a backpatch later if there is >> more ask for it, as well. > > Daniel, are you planning to apply this one on HEAD? I had another look over this and pushed it. -- Daniel Gustafsson https://vmware.com/
Re: [EXTERNAL] Re: Support load balancing in libpq
+1 for overall idea of load balancing via random host selection. For the patch itself, I think it is better to use a more precise time function in libpq_prng_init or call it only once. Thought it is a special corner case, imagine all the connection attempts at first second will be seeded with the save value, i.e. will attempt to connect to the same host. I think, this is not we want to achieve. And the "hostroder" option should be free'd in freePGconn. > Also, IMO, the solution must have a fallback mechanism if the > standby/chosen host isn't reachable. Yeah, I think it should. I'm not insisting on a particular name of options here, but in my view, the overall idea may be next: - we have two libpq's options: "load_balance_hosts" and "failover_timeout"; - the "load_balance_hosts" should be "sequential" or "random"; - the "failover_timeout" is a time period, within which, if connection to the server is not established, we switch to the next address or host. While writing this text, I start thinking that load balancing is a combination of two parameters above. > 3) Isn't it good to provide a way to test the patch? Good idea too. I think, we should add tap test here. -- Best regards, Maxim Orlov.
Re: Refactoring postgres_fdw/connection.c
On 2022/09/05 15:17, Etsuro Fujita wrote:
+1 for that refactoring. Here are a few comments about the 0001 patch:
Thanks for reviewing the patch!
I'm not sure it's a good idea to change the function's name, because
that would make backpatching hard. To avoid that, how about
introducing a workhorse function for pgfdw_get_result and
pgfdw_get_cleanup_result, based on the latter function as you
proposed, and modifying the two functions so that they call the
workhorse function?
That's possible. We can revive pgfdw_get_cleanup_result() and
make it call pgfdw_get_result_timed(). Also, with the patch,
pgfdw_get_result() already works in that way. But I'm not sure
how much we should be concerned about back-patch "issue"
in this case. We usually rename the internal functions
if new names are better.
@@ -1599,13 +1572,9 @@ pgfdw_finish_pre_commit_cleanup(List *pending_entries)
entry = (ConnCacheEntry *) lfirst(lc);
/* Ignore errors (see notes in pgfdw_xact_callback) */
- while ((res = PQgetResult(entry->conn)) != NULL)
- {
- PQclear(res);
- /* Stop if the connection is lost (else we'll loop infinitely) */
- if (PQstatus(entry->conn) == CONNECTION_BAD)
- break;
- }
+ pgfdw_get_result_timed(entry->conn, 0, &res, NULL);
+ PQclear(res);
The existing code prevents interruption, but this would change to
allow it. Do we need this change?
You imply that we intentially avoided calling CHECK_FOR_INTERRUPT()
there, don't you? But could you tell me why?
I have to agree with Horiguchi-san, because as mentioned by him, 1)
there isn't enough duplicate code in the two bits to justify merging
them into a single function, and 2) the 0002 patch rather just makes
code complicated. The current implementation is easy to understand,
so I'd vote for leaving them alone for now.
(I think the introduction of pgfdw_abort_cleanup is good, because that
de-duplicated much code that existed both in pgfdw_xact_callback and
in pgfdw_subxact_callback, which would outweigh the downside of
pgfdw_abort_cleanup that it made code somewhat complicated due to the
logic to handle both the transaction and subtransaction cases within
that single function. But 0002 is not the case, I think.)
The function pgfdw_exec_pre_commit() that I newly introduced consists
of two parts; issue the transaction-end command based on
parallel_commit setting and issue DEALLOCATE ALL. The first part is
duplicated between pgfdw_xact_callback() and pgfdw_subxact_callback(),
but the second not (i.e., it's used only by pgfdw_xact_callback()).
So how about getting rid of that non duplicated part from
pgfdw_exec_pre_commit()?
It gives the same feeling with 0002.
I have to agree with Horiguchi-san on this as well; the existing
single-purpose functions are easy to understand, so I'd vote for
leaving them alone.
Ok, I will reconsider 0003 patch. BTW, parallel abort patch that
you're proposing seems to add new function pgfdw_finish_abort_cleanup()
with the similar structure as the function added by 0003 patch.
So probably it's helpful for us to consider this together :)
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Counterintuitive behavior when toast_tuple_target < TOAST_TUPLE_THRESHOLD
Hi hackers, Recently in one discussion a user complained [1] about counterintuitive behavior of toast_tuple_target. Here is a quote: """ Table size 177.74 GB Toast table size 12 GB Indexes size 33.49 GB This table is composed of small columns "id", "hash", "size", and a mid~big (2~512kb) jsonb. I don't want to be forced to read the big column when doing seq scans, so I tried to set toast_tuple_target = 128, to exclude the big column, but even after a VACUUM FULL i couldn't get pg to toast the big column. Am I doing something wrong? """ Arguably in this case the user may actually want to store the JSONB fields by the foreign key. However the user may have a good point that setting toast_tuple_target < TOAST_TUPLE_THRESHOLD effectively does nothing. This happens because [2]: """ The TOAST management code is triggered only when a row value to be stored in a table is wider than TOAST_TUPLE_THRESHOLD bytes (normally 2 kB). The TOAST code will compress and/or move field values out-of-line until the row value is shorter than toast_tuple_target bytes (also normally 2 kB, adjustable) or no more gains can be had. """ ... TOAST is _triggered_ by TOAST_TUPLE_THRESHOLD but tries to compress the tuple until toast_tuple_target bytes. This is indeed somewhat confusing. I see several ways of solving this. 1. Forbid setting toast_tuple_target < TOAST_TUPLE_THRESHOLD 2. Consider using something like RelationGetToastTupleTarget(rel, TOAST_TUPLE_THRESHOLD) in heapam.c:2250, heapam.c:3625 and rewriteheap.c:636 and modify the documentation accordingly. 3. Add a separate user-defined table setting toast_tuple_threshold similar to toast_tuple_target. Thoughts? [1]: https://t.me/pg_sql/62265 [2]: https://www.postgresql.org/docs/current/storage-toast.html -- Best regards, Aleksander Alekseev
Fix comment in convert_saop_to_hashed_saop
In 29f45e29 we added support for executing NOT IN(values) with a hashtable, however this comment still claims that we only do so for cases where the ScalarArrayOpExpr's useOr flag is true. See attached for fix. Thanks, James Coleman v1-0001-Fix-convert_saop_to_hashed_saop-comment.patch Description: Binary data
Re: Counterintuitive behavior when toast_tuple_target < TOAST_TUPLE_THRESHOLD
Hi hackers, > 1. Forbid setting toast_tuple_target < TOAST_TUPLE_THRESHOLD Reading my own email I realized that this of course was stupid. For sure this is not an option. It's getting late in my timezone, sorry :) -- Best regards, Aleksander Alekseev
Re: New strategies for freezing, advancing relfrozenxid early
On Wed, Sep 14, 2022 at 3:18 AM John Naylor wrote: > On Wed, Sep 14, 2022 at 12:53 AM Peter Geoghegan wrote: > > This is still only scratching the surface of what is possible with > > dead_items. The visibility map snapshot concept can enable a far more > > sophisticated approach to resource management in vacuumlazy.c. > I don't quite see how it helps "enable" that. I have already written a simple throwaway patch that can use the current VM snapshot data structure (which is just a local copy of the VM's pages) to do a cheap precheck ahead of actually doing a binary search in dead_items -- if a TID's heap page is all-visible or all-frozen (depending on the type of VACUUM) then we're 100% guaranteed to not visit it, and so it's 100% guaranteed to not have any dead_items (actually it could have LP_DEAD items by the time the index scan happens, but they won't be in our dead_items array in any case). Since we're working off of an immutable source, this optimization is simple to implement already. Very simple. I haven't even bothered to benchmark this throwaway patch (I literally wrote it in 5 minutes to show Masahiko what I meant). I can't see why even that throwaway prototype wouldn't significantly improve performance, though. After all, the VM snapshot data structure is far denser than dead_items, and the largest tables often have most heap pages skipped via the VM. I'm not really interested in pursuing this simple approach because it conflicts with Masahiko's work on the data structure, and there are other good reasons to expect that to help. Plus I'm already very busy with what I have here. > It'd be more logical to > me to say the VM snapshot *requires* you to think harder about > resource management, since a palloc'd snapshot should surely be > counted as part of the configured memory cap that admins control. That's clearly true -- it creates a new problem for resource management that will need to be solved. But that doesn't mean that it can't ultimately make resource management better and easier. Remember, we don't randomly visit some skippable pages for no good reason in the patch, since the SKIP_PAGES_THRESHOLD stuff is completely gone. The VM snapshot isn't just a data structure that vacuumlazy.c uses as it sees fit -- it's actually more like a set of instructions on which pages to scan, that vacuumlazy.c *must* follow. There is no way that vacuumlazy.c can accidentally pick up a few extra dead_items here and there due to concurrent activity that unsets VM pages. We don't need to leave that to chance -- it is locked in from the start. > I do remember your foreshadowing in the radix tree thread a while > back, and I do think it's an intriguing idea to combine pages-to-scan > and dead TIDs in the same data structure. The devil is in the details, > of course. It's worth looking into. Of course. > Looking at the count of index scans, it's pretty much always > "1", so even if the current approach could scale above 1GB, "no" it > wouldn't help to raise that limit. I agree that multiple index scans are rare. But I also think that they're disproportionately involved in really problematic cases for VACUUM. That said, I agree that simply making lookups to dead_items as fast as possible is the single most important way to improve VACUUM by improving dead_items. > Furthermore, it doesn't have to anticipate the maximum size, so there > is no up front calculation assuming max-tuples-per-page, so it > automatically uses less memory for less demanding tables. The final number of TIDs doesn't seem like the most interesting information that VM snapshots could provide us when it comes to building the dead_items TID data structure -- the *distribution* of TIDs across heap pages seems much more interesting. The "shape" can be known ahead of time, at least to some degree. It can help with compression, which will reduce cache misses. Andres made remarks about memory usage with sparse dead TID patterns at this point on the "Improve dead tuple storage for lazy vacuum" thread: https://postgr.es/m/20210710025543.37sizjvgybemk...@alap3.anarazel.de I haven't studied the radix tree stuff in great detail, so I am uncertain of how much the VM snapshot concept could help, and where exactly it would help. I'm just saying that it seems promising, especially as a way of addressing concerns like this. -- Peter Geoghegan
Re: Allow logical replication to copy tables in binary format
Hi hackers, I just wanted to gently ping to hear what you all think about this patch. Appreciate any feedback/thougths. Thanks, Melih
Re: [RFC] building postgres with meson - v12
On 07.09.22 09:53, Peter Eisentraut wrote: On 07.09.22 09:19, Peter Eisentraut wrote: This could also be combined with the idea of the postgres.sgml.valid thing you have in the meson patch set. I'll finish this up and produce a proper patch. Something like this. This does make the rules more straightforward and avoids repeated xmllint calls. I suppose this also helps writing the meson rules in a simpler way. committed this
Re: [RFC] building postgres with meson - v12
On 08.09.22 09:42, Alvaro Herrera wrote: On 2022-Sep-07, Peter Eisentraut wrote: A possible drawback is that the intermediate postgres-full.xml file is 10MB, but I guess we're past the point where we are worrying about that kind of thing. I think we are, but maybe mark it .PRECIOUS? IIUC that would prevent it from being removed if there's a problem in the other recipes. I don't think .PRECIOUS is the right tool here. There are existing uses of .SECONDARY in doc/src/sgml/Makefile; I integrated my patch there.
Re: archive modules
On 14.09.22 07:25, Michael Paquier wrote: removed or recycled until they are archived. If WAL archiving cannot keep up - with the pace that WAL is generated, or if archive_command + with the pace that WAL is generated, or if archive_library fails repeatedly, old WAL files will accumulate in pg_wal with removed or recycled until they are archived. If WAL archiving cannot keep up with the pace that WAL is generated, or if archive_command with the pace that WAL is generated, or if archive_command or archive_library fail repeatedly, old WAL files will accumulate in pg_wal Yep. Some references to archive_library have been changed by 31e121 to do exactly that. There seem to be more spots in need of an update. I don't see anything in 31e121 about that. Here is a patch that addresses this. From 51512cd9cb59d169b041d10d62fc6a282011675c Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Wed, 14 Sep 2022 21:26:10 +0200 Subject: [PATCH] Restore archive_command documentation Commit 5ef1eefd76f404ddc59b885d50340e602b70f05f, which added archive_library, purged most mentions of archive_command from the documentation. This is inappropriate, since archive_command is still a feature in use and users will want to see information about it. This restores all the removed mentions and rephrases things so that archive_command and archive_library are presented as alternatives of each other. --- doc/src/sgml/backup.sgml| 50 + doc/src/sgml/config.sgml| 58 +++-- doc/src/sgml/high-availability.sgml | 6 +-- doc/src/sgml/ref/pg_receivewal.sgml | 7 +++- doc/src/sgml/wal.sgml | 3 +- 5 files changed, 76 insertions(+), 48 deletions(-) diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml index dee59bb422..a6d7105836 100644 --- a/doc/src/sgml/backup.sgml +++ b/doc/src/sgml/backup.sgml @@ -593,7 +593,7 @@ Setting Up WAL Archiving provide the database administrator with flexibility, PostgreSQL tries not to make any assumptions about how the archiving will be done. Instead, PostgreSQL lets -the administrator specify an archive library to be executed to copy a +the administrator specify a shell command or an archive library to be executed to copy a completed segment file to wherever it needs to go. This could be as simple as a shell command that uses cp, or it could invoke a complex C function — it's all up to you. @@ -603,13 +603,15 @@ Setting Up WAL Archiving To enable WAL archiving, set the configuration parameter to replica or higher, to on, -and specify the library to use in the configuration parameter +or specify the library to use in the configuration parameter. In practice these settings will always be placed in the postgresql.conf file. -One simple way to archive is to set archive_library to -an empty string and to specify a shell command in -. + + + In archive_command, %p is replaced by the path name of the file to archive, while %f is replaced by only the file name. @@ -633,6 +635,24 @@ Setting Up WAL Archiving A similar command will be generated for each new file to be archived. + +It is important that the archive command return zero exit status if and +only if it succeeds. Upon getting a zero result, +PostgreSQL will assume that the file has been +successfully archived, and will remove or recycle it. However, a nonzero +status tells PostgreSQL that the file was not archived; +it will try again periodically until it succeeds. + + + +When the archive command is terminated by a signal (other than +SIGTERM that is used as part of a server +shutdown) or an error by the shell with an exit status greater than +125 (such as command not found), the archiver process aborts and gets +restarted by the postmaster. In such cases, the failure is +not reported in . + + Another way to archive is to use a custom archive module as the archive_library. Since such modules are written in @@ -678,7 +698,7 @@ Setting Up WAL Archiving -The archive library should generally be designed to refuse to overwrite +Archive commands and libraries should generally be designed to refuse to overwrite any pre-existing archive file. This is an important safety feature to preserve the integrity of your archive in case of administrator error (such as sending the output of two different servers to the same archive @@ -686,9 +706,9 @@ Setting Up WAL Archiving -It is advisable to test your proposed archive library to ensure that it +It is advisable to test your proposed archive command or library to ensure that it indeed does not overwrite an existing file, and that it returns -false in this case. +nonzero status or false, respectively,
Re: Counterintuitive behavior when toast_tuple_target < TOAST_TUPLE_THRESHOLD
Hi! I've noticed this behavior half a year ago during experiments with TOAST, and TOAST_TUPLE_THRESHOLD really works NOT the way it is thought to. I propose something like FORCE_TOAST flag/option as column option (stored in attoptions), because we already encountered multiple cases where data should be stored externally despite its size. Currently I'm working on passing Toaster options in attoptions. Thoughts? On Wed, Sep 14, 2022 at 7:12 PM Aleksander Alekseev < aleksan...@timescale.com> wrote: > Hi hackers, > > > 1. Forbid setting toast_tuple_target < TOAST_TUPLE_THRESHOLD > > Reading my own email I realized that this of course was stupid. For > sure this is not an option. It's getting late in my timezone, sorry :) > > -- > Best regards, > Aleksander Alekseev > > > -- Regards, Nikita Malakhov Postgres Professional https://postgrespro.ru/
Re: archive modules
Another question on this feature: Currently, if archive_library is set, archive_command is ignored. I think if both are set, it should be an error. Compare for example what happens if you set multiple recovery_target_xxx settings. I don't think silently turning off one setting by setting another is a good behavior.
Re: why can't a table be part of the same publication as its schema
On 14.09.22 07:10, houzj.f...@fujitsu.com wrote: After applying the patch, we support adding a table with column list along with the table's schema[1], and it will directly apply the column list in the logical replication after applying the patch. [1]-- CREATE PUBLICATION pub FOR TABLE public.test(a), FOR ALL TABLES IN SCHEMA public; - If from the point of view of consistency, for column list, we could report an ERROR because we currently don't allow using different column lists for a table. Maybe an ERROR like: "ERROR: cannot use column for table x when the table's schema is also in the publication" But if we want to report an ERROR for column list in above case. We might need to restrict the ALTER TABLE SET SCHEMA as well because user could move a table which is published with column list to a schema that is also published in the publication, so we might need to add some similar check(which is removed in Vignesh's patch) to tablecmd.c to disallow this case. Another option could be just ingore the column list if table's schema is also part of publication. But it seems slightly inconsistent with the rule that we disallow using different column list for a table. Ignoring things doesn't seem like a good idea. A solution might be to disallow adding any schemas to a publication if column lists on a table are specified.
Re: archive modules
On Wed, Sep 14, 2022 at 09:33:46PM +0200, Peter Eisentraut wrote: > Another question on this feature: Currently, if archive_library is set, > archive_command is ignored. I think if both are set, it should be an error. > Compare for example what happens if you set multiple recovery_target_xxx > settings. I don't think silently turning off one setting by setting another > is a good behavior. I originally did it this way, but changed it based on this feedback [0]. I have no problem with the general idea, but the recovery_target_* logic does have the following note: * XXX this code is broken by design. Throwing an error from a GUC assign * hook breaks fundamental assumptions of guc.c. So long as all the variables * for which this can happen are PGC_POSTMASTER, the consequences are limited, * since we'd just abort postmaster startup anyway. Nonetheless it's likely * that we have odd behaviors such as unexpected GUC ordering dependencies. [0] https://postgr.es/m/CA%2BTgmoaf4Y7_U%2B_W%2BSg5DoAta_FMssr%3D52mx7-_tJnfaD1VubQ%40mail.gmail.com -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: cataloguing NOT NULL constraints
On 09.09.22 19:58, Alvaro Herrera wrote: There were a lot more problems in that submission than I at first realized, and I had to rewrite a lot of code in order to fix them. I have fixed all the user-visible problems I found in this version, and reviewed the tests results more carefully so I am now more confident that behaviourally it's doing the right thing; but Reading through the SQL standard again, I think this patch goes a bit too far in folding NOT NULL and CHECK constraints together. The spec says that you need to remember whether a column was defined as NOT NULL, and that the commands DROP NOT NULL and SET NOT NULL only affect constraints defined in that way. In this implementation, a constraint defined as NOT NULL is converted to a CHECK (x IS NOT NULL) constraint and the original definition is forgotten. Besides that, I think that users are not going to like that pg_dump rewrites their NOT NULL constraints into CHECK table constraints. I suspect that this needs a separate contype for NOT NULL constraints that is separate from CONSTRAINT_CHECK.
Re: archive modules
On 14.09.22 22:03, Nathan Bossart wrote: On Wed, Sep 14, 2022 at 09:33:46PM +0200, Peter Eisentraut wrote: Another question on this feature: Currently, if archive_library is set, archive_command is ignored. I think if both are set, it should be an error. Compare for example what happens if you set multiple recovery_target_xxx settings. I don't think silently turning off one setting by setting another is a good behavior. I originally did it this way, but changed it based on this feedback [0]. I have no problem with the general idea, but the recovery_target_* logic does have the following note: * XXX this code is broken by design. Throwing an error from a GUC assign * hook breaks fundamental assumptions of guc.c. So long as all the variables * for which this can happen are PGC_POSTMASTER, the consequences are limited, * since we'd just abort postmaster startup anyway. Nonetheless it's likely * that we have odd behaviors such as unexpected GUC ordering dependencies. Ah yes, that won't work. But maybe we can just check it at run time, like in LoadArchiveLibrary().
Re: archive modules
Peter Eisentraut writes: > On 14.09.22 22:03, Nathan Bossart wrote: >> I originally did it this way, but changed it based on this feedback [0]. I >> have no problem with the general idea, but the recovery_target_* logic does >> have the following note: >> >> * XXX this code is broken by design. Throwing an error from a GUC assign >> * hook breaks fundamental assumptions of guc.c. So long as all the variables >> * for which this can happen are PGC_POSTMASTER, the consequences are limited, >> * since we'd just abort postmaster startup anyway. Nonetheless it's likely >> * that we have odd behaviors such as unexpected GUC ordering dependencies. > Ah yes, that won't work. But maybe we can just check it at run time, > like in LoadArchiveLibrary(). Yeah, the objection there is only to trying to enforce such interrelationships in GUC hooks. In this case it seems to me that we could easily check and complain at the point where we're about to use the GUC values. regards, tom lane
Re: archive modules
On Wed, Sep 14, 2022 at 09:31:04PM +0200, Peter Eisentraut wrote: > Here is a patch that addresses this. My intent was to present archive_command as the built-in archive library, but I can see how this might cause confusion, so this change seems reasonable to me. > + > +It is important that the archive command return zero exit status if and > +only if it succeeds. Upon getting a zero result, > +PostgreSQL will assume that the file has been > +successfully archived, and will remove or recycle it. However, a nonzero > +status tells PostgreSQL that the file was not > archived; > +it will try again periodically until it succeeds. > + > + > + > +When the archive command is terminated by a signal (other than > +SIGTERM that is used as part of a server > +shutdown) or an error by the shell with an exit status greater than > +125 (such as command not found), the archiver process aborts and gets > +restarted by the postmaster. In such cases, the failure is > +not reported in . > + This wording is very similar to the existing wording in the archive library section below it. I think the second paragraph covers the shell command case explicitly, too. Perhaps these should be combined. > +archive_mode and > archive_command are > +separate variables so that archive_command can be > +changed without leaving archiving mode. I believe this applies to archive_library, too. > - for segments to complete like does. > + for segments to complete like and > +does. nitpick: s/does/do -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: archive modules
On Wed, Sep 14, 2022 at 04:47:23PM -0400, Tom Lane wrote: > Peter Eisentraut writes: >> On 14.09.22 22:03, Nathan Bossart wrote: >>> I originally did it this way, but changed it based on this feedback [0]. I >>> have no problem with the general idea, but the recovery_target_* logic does >>> have the following note: >>> >>> * XXX this code is broken by design. Throwing an error from a GUC assign >>> * hook breaks fundamental assumptions of guc.c. So long as all the >>> variables >>> * for which this can happen are PGC_POSTMASTER, the consequences are >>> limited, >>> * since we'd just abort postmaster startup anyway. Nonetheless it's likely >>> * that we have odd behaviors such as unexpected GUC ordering dependencies. > >> Ah yes, that won't work. But maybe we can just check it at run time, >> like in LoadArchiveLibrary(). > > Yeah, the objection there is only to trying to enforce such > interrelationships in GUC hooks. In this case it seems to me that > we could easily check and complain at the point where we're about > to use the GUC values. I think the cleanest way to do something like that would be to load a check_configured_cb that produces a WARNING. IIRC failing in LoadArchiveLibrary() would just cause the archiver process to restart over and over. HandlePgArchInterrupts() might need some work as well. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Fix comment in convert_saop_to_hashed_saop
On Thu, 15 Sept 2022 at 04:08, James Coleman wrote: > In 29f45e29 we added support for executing NOT IN(values) with a > hashtable, however this comment still claims that we only do so for > cases where the ScalarArrayOpExpr's useOr flag is true. > > See attached for fix. Thank you. Pushed. David
Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
Jelte Fennema writes:
> [ non-blocking PQcancel ]
I pushed the 0001 patch (libpq_pipeline documentation) with a bit
of further wordsmithing.
As for 0002, I'm not sure that's anywhere near ready. I doubt it's
a great idea to un-deprecate PQrequestCancel with a major change
in its behavior. If there is anybody out there still using it,
they're not likely to appreciate that. Let's leave that alone and
pick some other name.
I'm also finding the entire design of PQrequestCancelStart etc to
be horribly confusing --- it's not *bad* necessarily, but the chosen
function names are seriously misleading. PQrequestCancelStart doesn't
actually "start" anything, so the apparent parallel with PQconnectStart
is just wrong. It's also fairly unclear what the state of a cancel
PQconn is after the request cycle is completed, and whether you can
re-use it (especially after a failed request), and whether you have
to dispose of it separately.
On the whole it feels like a mistake to have two separate kinds of
PGconn with fundamentally different behaviors and yet no distinction
in the API. I think I'd recommend having a separate struct type
(which might internally contain little more than a pointer to a
cloned PGconn), and provide only a limited set of operations on it.
Seems like create, start/continue cancel request, destroy, and
fetch error message ought to be enough. I don't see a reason why we
need to support all of libpq's inquiry operations on such objects ---
for instance, if you want to know which host is involved, you could
perfectly well query the parent PGconn. Nor do I want to run around
and add code to every single libpq entry point to make it reject cancel
PGconns if it can't support them, but we'd have to do so if there's
just one struct type.
I'm not seeing the use-case for PQconnectComplete. If you want
a non-blocking cancel request, why would you then use a blocking
operation to complete the request? Seems like it'd be better
to have just a monolithic cancel function for those who don't
need non-blocking.
This change:
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -59,12 +59,15 @@ typedef enum
{
CONNECTION_OK,
CONNECTION_BAD,
+ CONNECTION_CANCEL_FINISHED,
/* Non-blocking mode only below here */
is an absolute non-starter: it breaks ABI for every libpq client,
even ones that aren't using this facility. Why do we need a new
ConnStatusType value anyway? Seems like PostgresPollingStatusType
covers what we need: once you reach PGRES_POLLING_OK, the cancel
request is done.
The test case is still not very bulletproof on slow machines,
as it seems to be assuming that 30 seconds == forever. It
would be all right to use $PostgreSQL::Test::Utils::timeout_default,
but I'm not sure that that's easily retrievable by C code.
Maybe make the TAP test pass it in with another optional switch
to libpq_pipeline? Alternatively, we could teach libpq_pipeline
to do getenv("PG_TEST_TIMEOUT_DEFAULT") with a fallback to 180,
but that feels like it might be overly familiar with the innards
of Utils.pm.
regards, tom lane
Re: [PATCH] Support % wildcard in extension upgrade filenames
I'm attaching an updated version of the patch. This time the patch is tested. Nothing changes unless the .control file for the subject extension doesn't have a "wildcard_upgrades = true" statement. When wildcard upgrades are enabled, a file with a "%" symbol as the "source" part of the upgrade path will match any version and will be used if a specific version upgrade does not exist. This means that in presence of the following files: postgis--3.0.0--3.2.0.sql postgis--%--3.2.0.sql The first one will be used for going from 3.0.0 to 3.2.0. This is the intention. The patch lacks automated tests and can probably be improved. For more context, a previous (non-working) version of this patch was submitted to commitfest: https://commitfest.postgresql.org/38/3654/ --strk; On Sat, Jun 04, 2022 at 11:20:55AM +0200, Sandro Santilli wrote: > On Sat, May 28, 2022 at 04:50:20PM +0200, Laurenz Albe wrote: > > On Fri, 2022-05-27 at 17:37 -0400, Regina Obe wrote: > > > > > > https://lists.osgeo.org/pipermail/postgis-devel/2022-February/029500.html > > > > > > Does anyone think this is such a horrible idea that we should abandon all > > > hope? > > > > I don't think this idea is fundamentally wrong, but I have two worries: > > > > 1. It would be a good idea good to make sure that there is not both > >"extension--%--2.0.sql" and "extension--1.0--2.0.sql" present. > >Otherwise the behavior might be indeterministic. > > I'd make sure to use extension--1.0--2.0.sql in that case (more > specific first). > > > 2. What if you have a "postgis--%--3.3.sql", and somebody tries to upgrade > >their PostGIS 1.1 installation with it? Would that work? > > For PostGIS in particular it will NOT work as the PostGIS upgrade > script checks for the older version and decides if the upgrade is > valid or not. This is the same upgrade code used for non-extension > installs. > > >Having a lower bound for a matching version might be a good idea, > >although I have no idea how to do that. > > I was thinking of a broader pattern matching support, like: > > postgis--3.%--3.3.sql > > But it would be better to start simple and eventually if needed > increase the complexity ? > > Another option could be specifying something in the control file, > which would also probably be a good idea to still allow some > extensions to use '%' in the version string (for example). > > --strk; > > Libre GIS consultant/developer > https://strk.kbt.io/services.html > >
Re: [PATCH] Support % wildcard in extension upgrade filenames
And now with the actual patch attached ... (sorry)
--strk;
On Thu, Sep 15, 2022 at 12:01:04AM +0200, Sandro Santilli wrote:
> I'm attaching an updated version of the patch. This time the patch
> is tested. Nothing changes unless the .control file for the subject
> extension doesn't have a "wildcard_upgrades = true" statement.
>
> When wildcard upgrades are enabled, a file with a "%" symbol as
> the "source" part of the upgrade path will match any version and
> will be used if a specific version upgrade does not exist.
> This means that in presence of the following files:
>
> postgis--3.0.0--3.2.0.sql
> postgis--%--3.2.0.sql
>
> The first one will be used for going from 3.0.0 to 3.2.0.
>
> This is the intention. The patch lacks automated tests and can
> probably be improved.
>
> For more context, a previous (non-working) version of this patch was
> submitted to commitfest: https://commitfest.postgresql.org/38/3654/
>
> --strk;
>
> On Sat, Jun 04, 2022 at 11:20:55AM +0200, Sandro Santilli wrote:
> > On Sat, May 28, 2022 at 04:50:20PM +0200, Laurenz Albe wrote:
> > > On Fri, 2022-05-27 at 17:37 -0400, Regina Obe wrote:
> > > >
> > > > https://lists.osgeo.org/pipermail/postgis-devel/2022-February/029500.html
> > > >
> > > > Does anyone think this is such a horrible idea that we should abandon
> > > > all
> > > > hope?
> > >
> > > I don't think this idea is fundamentally wrong, but I have two worries:
> > >
> > > 1. It would be a good idea good to make sure that there is not both
> > >"extension--%--2.0.sql" and "extension--1.0--2.0.sql" present.
> > >Otherwise the behavior might be indeterministic.
> >
> > I'd make sure to use extension--1.0--2.0.sql in that case (more
> > specific first).
> >
> > > 2. What if you have a "postgis--%--3.3.sql", and somebody tries to upgrade
> > >their PostGIS 1.1 installation with it? Would that work?
> >
> > For PostGIS in particular it will NOT work as the PostGIS upgrade
> > script checks for the older version and decides if the upgrade is
> > valid or not. This is the same upgrade code used for non-extension
> > installs.
> >
> > >Having a lower bound for a matching version might be a good idea,
> > >although I have no idea how to do that.
> >
> > I was thinking of a broader pattern matching support, like:
> >
> > postgis--3.%--3.3.sql
> >
> > But it would be better to start simple and eventually if needed
> > increase the complexity ?
> >
> > Another option could be specifying something in the control file,
> > which would also probably be a good idea to still allow some
> > extensions to use '%' in the version string (for example).
> >
> > --strk;
>From 5d57d7b755c3a23ca94bf922581a417844249044 Mon Sep 17 00:00:00 2001
From: Sandro Santilli
Date: Wed, 14 Sep 2022 11:10:10 +0200
Subject: [PATCH v2] Allow wildcard (%) in extension upgrade paths
A wildcard character "%" will be accepted in the
"source" side of the upgrade script and be considered
usable to upgrade any version to the "target" side.
Using wildcards needs to be explicitly requested by
extensions via a "wildcard_upgrades" setting in their
control file.
---
src/backend/commands/extension.c | 58
1 file changed, 52 insertions(+), 6 deletions(-)
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 6b6720c690..e36a79ae75 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -86,6 +86,7 @@ typedef struct ExtensionControlFile
bool relocatable; /* is ALTER EXTENSION SET SCHEMA supported? */
bool superuser; /* must be superuser to install? */
bool trusted; /* allow becoming superuser on the fly? */
+ bool wildcard_upgrades; /* allow using wildcards in upgrade scripts */
int encoding; /* encoding of the script file, or -1 */
List *requires; /* names of prerequisite extensions */
} ExtensionControlFile;
@@ -128,6 +129,7 @@ static void ApplyExtensionUpdates(Oid extensionOid,
bool cascade,
bool is_create);
static char *read_whole_file(const char *filename, int *length);
+static bool file_exists(const char *name);
/*
@@ -579,6 +581,14 @@ parse_extension_control_file(ExtensionControlFile *control,
errmsg("parameter \"%s\" requires a Boolean value",
item->name)));
}
+ else if (strcmp(item->name, "wildcard_upgrades") == 0)
+ {
+ if (!parse_bool(item->value, &control->wildcard_upgrades))
+ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("parameter \"%s\" requires a Boolean value",
+item->name)));
+ }
else if (strcmp(item->name, "encoding") == 0)
{
control->encoding = pg_valid_server_encoding(item->value);
@@ -636,6 +646,7 @@ read_extension_control_file(const char *extname)
control->relocatable = false;
control->superuser = true;
control->trusted = false;
+ control->wildcard_upgrades = false;
control->encoding = -1;
/*
@@ -890,7 +901
Re: archive modules
Nathan Bossart writes: > On Wed, Sep 14, 2022 at 04:47:23PM -0400, Tom Lane wrote: >> Yeah, the objection there is only to trying to enforce such >> interrelationships in GUC hooks. In this case it seems to me that >> we could easily check and complain at the point where we're about >> to use the GUC values. > I think the cleanest way to do something like that would be to load a > check_configured_cb that produces a WARNING. IIRC failing in > LoadArchiveLibrary() would just cause the archiver process to restart over > and over. HandlePgArchInterrupts() might need some work as well. Hm. Maybe consistency-check these settings in the postmaster, sometime after we've absorbed all GUC settings but before we launch any children? That could provide a saner implementation for the recovery_target_* variables too. regards, tom lane
Re: archive modules
On Wed, Sep 14, 2022 at 06:12:09PM -0400, Tom Lane wrote:
> Nathan Bossart writes:
>> On Wed, Sep 14, 2022 at 04:47:23PM -0400, Tom Lane wrote:
>>> Yeah, the objection there is only to trying to enforce such
>>> interrelationships in GUC hooks. In this case it seems to me that
>>> we could easily check and complain at the point where we're about
>>> to use the GUC values.
>
>> I think the cleanest way to do something like that would be to load a
>> check_configured_cb that produces a WARNING. IIRC failing in
>> LoadArchiveLibrary() would just cause the archiver process to restart over
>> and over. HandlePgArchInterrupts() might need some work as well.
>
> Hm. Maybe consistency-check these settings in the postmaster, sometime
> after we've absorbed all GUC settings but before we launch any children?
> That could provide a saner implementation for the recovery_target_*
> variables too.
Both archive_command and archive_library are PGC_SIGHUP, so IIUC that
wouldn't be sufficient. I attached a quick sketch that seems to provide
the desired behavior. It's nowhere near committable yet, but it
demonstrates what I'm thinking.
For recovery_target_*, something like you are describing seems reasonable.
I believe PostmasterMain() already performs some similar checks.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 6ce361707d..1d0c6029a5 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -422,8 +422,15 @@ pgarch_ArchiverCopyLoop(void)
HandlePgArchInterrupts();
/* can't do anything if not configured ... */
- if (ArchiveContext.check_configured_cb != NULL &&
-!ArchiveContext.check_configured_cb())
+ if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
+ {
+ereport(WARNING,
+ (errmsg("archive_mode enabled, but archiving is misconfigured"),
+ errdetail("Only one of archive_command, archive_library may be set.")));
+return;
+ }
+ else if (ArchiveContext.check_configured_cb != NULL &&
+ !ArchiveContext.check_configured_cb())
{
ereport(WARNING,
(errmsg("archive_mode enabled, yet archiving is not configured")));
@@ -794,6 +801,9 @@ HandlePgArchInterrupts(void)
{
char *archiveLib = pstrdup(XLogArchiveLibrary);
bool archiveLibChanged;
+ bool misconfiguredBeforeReload = (XLogArchiveCommand[0] != '\0' &&
+ XLogArchiveLibrary[0] != '\0');
+ bool misconfiguredAfterReload;
ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
@@ -801,7 +811,11 @@ HandlePgArchInterrupts(void)
archiveLibChanged = strcmp(XLogArchiveLibrary, archiveLib) != 0;
pfree(archiveLib);
- if (archiveLibChanged)
+ misconfiguredAfterReload = (XLogArchiveCommand[0] != '\0' &&
+ XLogArchiveLibrary[0] != '\0');
+
+ if ((archiveLibChanged && !misconfiguredAfterReload) ||
+ misconfiguredBeforeReload != misconfiguredAfterReload)
{
/*
* Call the currently loaded archive module's shutdown callback,
@@ -816,10 +830,17 @@ HandlePgArchInterrupts(void)
* internal_load_library()). To deal with this, we simply restart
* the archiver. The new archive module will be loaded when the
* new archiver process starts up.
+ *
+ * Similarly, we restart the archiver if our misconfiguration status
+ * changes. If the parameters were misconfigured but are no longer,
+ * we must restart to load the correct callbacks. If the parameters
+ * weren't misconfigured but now are, we must restart to unload the
+ * current callbacks.
*/
ereport(LOG,
(errmsg("restarting archiver process because value of "
- "\"archive_library\" was changed")));
+ "\"archive_library\" or \"archive_command\" was "
+ "changed")));
proc_exit(0);
}
@@ -838,6 +859,14 @@ LoadArchiveLibrary(void)
memset(&ArchiveContext, 0, sizeof(ArchiveModuleCallbacks));
+ /*
+ * If both a shell command and an archive library are specified, it is not
+ * clear what we should do, so do nothing. The archiver will emit WARNINGs
+ * about the misconfiguration.
+ */
+ if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
+ return;
+
/*
* If shell archiving is enabled, use our special initialization function.
* Otherwise, load the library and call its _PG_archive_module_init().
Re: Add support for DEFAULT specification in COPY FROM
On 2022-08-17 We 17:12, Israel Barth Rubio wrote: > Hello Andrew, > > Thanks for reviewing this patch [...] > > I am attaching the new patch, containing the above test in the regress > suite. > Thanks, this looks good but there are some things that need attention: . There needs to be a check that this is being used with COPY FROM, and the restriction needs to be stated in the docs and tested for. c.f. FORCE NULL. . There needs to be support for this in psql's tab_complete.c, and appropriate tests added . There needs to be support for it in contrib/file_fdw/file_fdw.c, and a test added . The tests should include psql's \copy as well as sql COPY . I'm not sure we need a separate regression test file for this. Probably these tests can go at the end of src/test/regress/sql/copy2.sql. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Use "WAL segment" instead of "log segment" consistently in user-facing messages
Bharath Rupireddy writes: > Merged. PSA v8 patch set. Pushed, thanks. regards, tom lane
Avoid use deprecated Windows Memory API
Hi. According to: https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-localalloc "Note The local functions have greater overhead and provide fewer features than other memory management functions. New applications should use the heap functions unless documentation states that a local function should be used. For more information, see Global and Local Functions." LocalAlloc is deprecated. So use HeapAlloc instead, once LocalAlloc is an overhead wrapper to HeapAlloc. Attached a patch. regards, Ranier Vilela use-heapalloc-instead-deprecated-localalloc.patch Description: Binary data
Re: Counterintuitive behavior when toast_tuple_target < TOAST_TUPLE_THRESHOLD
On Thu, 15 Sept 2022 at 04:04, Aleksander Alekseev wrote: > 1. Forbid setting toast_tuple_target < TOAST_TUPLE_THRESHOLD > 2. Consider using something like RelationGetToastTupleTarget(rel, > TOAST_TUPLE_THRESHOLD) in heapam.c:2250, heapam.c:3625 and > rewriteheap.c:636 and modify the documentation accordingly. > 3. Add a separate user-defined table setting toast_tuple_threshold > similar to toast_tuple_target. > > Thoughts? There was some discussion on this problem in [1]. The problem with #2 is that if you look at heapam_relation_needs_toast_table(), it only decides if the toast table should be created based on (tuple_length > TOAST_TUPLE_THRESHOLD). So if you were to change the logic as you describe for #2 then there might not be a toast table during an INSERT/UPDATE. The only way to fix that would be to ensure that we reconsider if we should create a toast table or not when someone changes the toast_tuple_target reloption. That can't be done under ShareUpdateExclusiveLock, so we'd need to obtain an AccessExclusiveLock instead when changing the toast_tuple_target reloption. That might upset some people. The general direction of [1] was to just increase the minimum setting to TOAST_TUPLE_THRESHOLD, but there were some concerns about breaking pg_dump as we'd have to error if someone does ALTER TABLE to set the toast_tuple_target reloption lower than the newly defined minimum value. I don't quite follow you on #3. If there's no toast table we can't toast. David [1] https://www.postgresql.org/message-id/20190403063759.gf3...@paquier.xyz
Re: [PATCH]Feature improvement for MERGE tab completion
On 2022-09-14 18:12, bt22kawamotok wrote:
I fixed it in v6.
Thanks for updating.
+ COMPLETE_WITH("UPDATE", "DELETE", "DO NOTHING");
"UPDATE" is always followed by "SET", so why not complement it with
"UPDATE SET"?
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: pgsql: Doc: Explain about Column List feature.
On Wed, Sep 14, 2022 at 7:40 PM Alvaro Herrera wrote: > > On 2022-Sep-14, Peter Smith wrote: > > > PSA a new patch for the "Column Lists" page. AFAIK this is the same as > > everything that you suggested > > I don't get it. You send me my patch back, and claim it is a new patch? > > I kindly request that when you review a patch, you do not hijack the > submitter's patch and claim it as your own. If a submitter goes mising > or states that they're unavailable to complete some work, then that's > okay, but otherwise it seems a bit offensive to me. I have seen that > repeatedly of late, and I find it quite rude. Hi Alvaro, I'm sorry for any misunderstandings. I attached the replacement patch primarily because the original did not apply for me, so I had to re-make it at my end anyway so I could see the result. I thought posting it might save others from having to do the same. Certainly I am not trying to hijack or claim ownership. -- Kind Regards, Peter Smith. Fujitsu Australia
RE: why can't a table be part of the same publication as its schema
On Thursday, September 15, 2022 3:37 AM Peter Eisentraut wrote: Hi, > > On 14.09.22 07:10, houzj.f...@fujitsu.com wrote: > > After applying the patch, we support adding a table with column list > > along with the table's schema[1], and it will directly apply the > > column list in the logical replication after applying the patch. > > > > [1]-- > > CREATE PUBLICATION pub FOR TABLE public.test(a), FOR ALL TABLES IN > > SCHEMA public; > > - > > > > If from the point of view of consistency, for column list, we could > > report an ERROR because we currently don't allow using different > > column lists for a table. Maybe an ERROR like: > > > > "ERROR: cannot use column for table x when the table's schema is also in the > publication" > > > > But if we want to report an ERROR for column list in above case. We > > might need to restrict the ALTER TABLE SET SCHEMA as well because user > > could move a table which is published with column list to a schema > > that is also published in the publication, so we might need to add > > some similar check(which is removed in Vignesh's patch) to tablecmd.c to > disallow this case. > > > > Another option could be just ingore the column list if table's schema > > is also part of publication. But it seems slightly inconsistent with > > the rule that we disallow using different column list for a table. > > Ignoring things doesn't seem like a good idea. > > A solution might be to disallow adding any schemas to a publication if column > lists on a table are specified. Thanks for the suggestion. If I understand correctly, you mean we can disallow publishing a table with column list and any schema(a schema that the table might not be part of) in the same publication[1]. something like-- [1]CREATE PUBLICATION pub FOR TABLE public.test(a), ALL TABLES IN SCHEMA s2; ERROR: "cannot add schema to publication when column list is used in the published table" -- Personally, it looks acceptable to me as user can anyway achieve the same purpose by creating serval publications and combine it and we can save the restriction at ALTER TABLE SET SCHEMA. Although it restricts some cases. I will post a top-up patch about this soon. About the row filter handling, maybe we don't need to restrict row filter like above ? Because the rule is to simply merge the row filter with 'OR' among publications, so it seems we could ignore the row filter in the publication when the table's schema is also published in the same publication(which means no filter). Best regards, Hou zj
Re: failing to build preproc.c on solaris with sun studio
On Wed, Sep 14, 2022 at 4:34 PM Justin Pryzby wrote:
> On Wed, Sep 14, 2022 at 03:08:06PM +1200, Thomas Munro wrote:
> > On Wed, Sep 14, 2022 at 10:23 AM Thomas Munro
> > wrote:
> > > Given the simplicity of this case, though, I suppose we could
> > > have a little not-very-general shell/python/whatever wrapper script --
> > > just compute a checksum of the input and keep the output files around.
> >
> > Something as dumb as this perhaps...
>
> > if [ -z "$c_file" ] ; then
> > c_file="(echo "$y_file" | sed 's/\.y/.tab.c/')"
> > fi
>
> This looks wrong. I guess you mean to use $() and missing "$" ?
Yeah, but your %.y style is much nicer. Fixed that way. (I was
trying to avoid what I thought were non-standard extensions but I see
that's in POSIX sh. Cool.)
> It could be:
> [ -z "$c_file" ] &&
> c_file=${y_file%.y}.tab.c
Meh.
> > if [ -z "$SIMPLE_BISON_CACHE_PATH" ] ; then
> > SIMPLE_BISON_CACHE_PATH="/tmp/simple-bison-cache"
> > fi
>
> Should this default to CCACHE_DIR? Then it would work under cirrusci...
Not sure it's OK to put random junk in ccache's directory, and in any
case we'd certainly want to teach it to trim itself before doing that
on CI... On the other hand, adding another registered cache dir would
likely add several seconds to CI, more than what can be saved with
this trick! The amount of time we can save is only a few seconds, or
less on a fast machine.
So... I guess the target audience of this script is extremely
impatient people working locally, since with Meson our clean builds
are cleaner, and will lead to re-execution this step. I just tried
Andres's current meson branch on my fast-ish 16 core desktop, and
then, after priming caches, "ninja clean && time ninja" tells me:
real0m3.133s
After doing 'meson configure
-DBISON="/path/to/simple-bison-cache.sh"', I get it down to:
real0m2.440s
However, in doing that I realised that you need an executable name,
not a hairy shell command fragment, so you can't use
"simple-bison-cache.sh bison", so I had to modify the script to be a
wrapper that knows how to find bison. Bleugh.
> > h_file="$(echo $c_file | sed 's/\.c/.h/')"
>
> Could be ${c_file%.c}.h
Much nicer.
> > if [ ! -e "$cached_c_file" -o ! -e "$cached_h_file" ] ; then
>
> You could write the easy case first (I forget whether it's considered to
> be more portable to write && outside of []).
Agreed, it's nicer that way around.
> I can't see what part of this would fail to handle filenames with spaces> (?)
Yeah, seems OK. I also fixed the uncertainty about -d, and made a
small tweak after testing on Debian, MacOS and FreeBSD. BTW this
isn't a proposal for src/tools yet, I'm just sharing for curiosity...
I suppose a version good enough to live in src/tools would need to
trim the cache, and I don't enjoy writing code that deletes files in
shell script, so maybe this'd need to be written in Python...
simple-bison-cache.sh
Description: application/shellscript
Re: Switching XLog source from archive to streaming when primary available
On Mon, Sep 12, 2022 at 11:56 AM Bharath Rupireddy wrote: > > Please review the attached v5 patch. I'm attaching the v6 patch that's rebased on to the latest HEAD. Please consider this for review. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com v6-0001-Allow-standby-to-switch-WAL-source-from-archive-t.patch Description: Binary data
Re: add log messages when replication slots become active and inactive (was Re: Is it worth adding ReplicationSlot active_pid to ReplicationSlotPersistentData?)
On Mon, Jul 25, 2022 at 6:31 PM Bharath Rupireddy wrote: > > Here's the v6 patch, a much simpler one - no changes to any of the > existing function APIs. Please see the sample logs at [1]. There's a > bit of duplicate code in the v6 patch, if the overall approach looks > okay, I can remove that too in the next version of the patch. I modified the log_replication_commands description in guc_tables.c. Please review the v7 patch further. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com v7-0001-Add-LOG-messages-when-replication-slots-become-ac.patch Description: Binary data
Re: [RFC] building postgres with meson - v13
Andres Freund writes: > I'm inclined to build the static lib on windows as long as we do it on other > platforms. Maybe I spent too much time working for Red Hat, but I'm kind of unhappy that we build static libraries at all. They are maintenance hazards and therefore security hazards by definition, because if you find a problem in $package_x you will have to find and rebuild every other package that has statically-embedded code from $package_x. So Red Hat has, or least had, a policy against packages exporting such libraries. I realize that there are people for whom other considerations outweigh that, but I don't think that we should install static libraries by default. Long ago it was pretty common for configure scripts to offer --enable-shared and --enable-static options ... should we resurrect that? regards, tom lane
RE: Perform streaming logical transactions by background workers and parallel apply
On Thur, Sep 8, 2022 at 19:25 PM Amit Kapila wrote: > On Thu, Sep 8, 2022 at 12:21 PM Amit Kapila wrote: > > > > On Mon, Sep 5, 2022 at 6:34 PM houzj.f...@fujitsu.com > > wrote: > > > > > > Attach the correct patch set this time. > > > > > > > Few comments on v28-0001*: > > === > > > > Some suggestions for comments in v28-0001* Thanks for your comments and patch! > 1. > +/* > + * Entry for a hash table we use to map from xid to the parallel apply worker > + * state. > + */ > +typedef struct ParallelApplyWorkerEntry > > Let's change this comment to: "Hash table entry to map xid to the > parallel apply worker state." > > 2. > +/* > + * List that stores the information of parallel apply workers that were > + * started. Newly added worker information will be removed from the list at > the > + * end of the transaction when there are enough workers in the pool. Besides, > + * exited workers will be removed from the list after being detected. > + */ > +static List *ParallelApplyWorkersList = NIL; > > Can we change this to: "A list to maintain the active parallel apply > workers. The information for the new worker is added to the list after > successfully launching it. The list entry is removed at the end of the > transaction if there are already enough workers in the worker pool. > For more information about the worker pool, see comments atop > worker.c. We also remove the entry from the list if the worker is > exited due to some error." > > Apart from this, I have added/changed a few other comments in > v28-0001*. Kindly check the attached, if you are fine with it then > please include it in the next version. Improved as suggested. The new patches were attached in [1]. [1] - https://www.postgresql.org/message-id/OS3PR01MB6275F145878B4A44586C46CE9E499%40OS3PR01MB6275.jpnprd01.prod.outlook.com Regards, Wang wei
Re: [RFC] building postgres with meson - v13
Hi, On 2022-09-15 01:10:16 -0400, Tom Lane wrote: > Andres Freund writes: > > I'm inclined to build the static lib on windows as long as we do it on other > > platforms. > > Maybe I spent too much time working for Red Hat, but I'm kind of > unhappy that we build static libraries at all. Yea, I have been wondering about that too. Oddly enough, given our current behaviour, the strongest case for static libraries IMO is on windows, due to the lack of a) rpath b) a general library search path. Peter IIRC added the static libraries to the meson port just to keep the set of installed files the same, which makes sense. > They are maintenance hazards and therefore security hazards by definition, > because if you find a problem in $package_x you will have to find and > rebuild every other package that has statically-embedded code from > $package_x. So Red Hat has, or least had, a policy against packages > exporting such libraries. It obviously is a bad idea for widely used system packages. I think there are a few situations, e.g. a downloadable self-contained and relocatable application, where shared libraries provide less of a benefit. > I realize that there are people for whom other considerations outweigh > that, but I don't think that we should install static libraries by > default. Long ago it was pretty common for configure scripts to > offer --enable-shared and --enable-static options ... should we > resurrect that? It'd be easy enough. I don't really have an opinion on whether it's worth having the options. I think most packaging systems have ways of not including files even if $software installs them. Greetings, Andres Freund
RE: Perform streaming logical transactions by background workers and parallel apply
On Fri, Sep 9, 2022 at 15:02 PM Peter Smith wrote:
> Here are my review comments for the v28-0001 patch:
>
> (There may be some overlap with other people's review comments and/or
> some fixes already made).
Thanks for your comments.
> 5. src/backend/libpq/pqmq.c
>
> + {
> + if (IsParallelWorker())
> + SendProcSignal(pq_mq_parallel_leader_pid,
> +PROCSIG_PARALLEL_MESSAGE,
> +pq_mq_parallel_leader_backend_id);
> + else
> + {
> + Assert(IsLogicalParallelApplyWorker());
> + SendProcSignal(pq_mq_parallel_leader_pid,
> +PROCSIG_PARALLEL_APPLY_MESSAGE,
> +pq_mq_parallel_leader_backend_id);
> + }
> + }
>
> This code can be simplified if you want to. For example,
>
> {
> ProcSignalReason reason;
> Assert(IsParallelWorker() || IsLogicalParallelApplyWorker());
> reason = IsParallelWorker() ? PROCSIG_PARALLEL_MESSAGE :
> PROCSIG_PARALLEL_APPLY_MESSAGE;
> SendProcSignal(pq_mq_parallel_leader_pid, reason,
>pq_mq_parallel_leader_backend_id);
> }
Not sure this would be better.
> 14.
>
> + /* Failed to start a new parallel apply worker. */
> + if (winfo == NULL)
> + return;
>
> There seem to be quite a lot of places (like this example) where
> something may go wrong and the behaviour apparently will just silently
> fall-back to using the non-parallel streaming. Maybe that is OK, but I
> am just wondering how can the user ever know this has happened? Maybe
> the docs can mention that this could happen and give some description
> of what processes users can look for (or some other strategy) so they
> can just confirm that the parallel streaming is really working like
> they assume it to be?
I think user could refer to the view pg_stat_subscription to check if the
parallel apply worker started.
BTW, we have documented the case if no parallel worker are available.
> 17. src/backend/replication/logical/applyparallelworker.c -
> parallel_apply_free_worker
>
> +/*
> + * Remove the parallel apply worker entry from the hash table. And stop the
> + * worker if there are enough workers in the pool.
> + */
> +void
> +parallel_apply_free_worker(ParallelApplyWorkerInfo *winfo, TransactionId
> xid)
>
> I think the reason for doing the "enough workers in the pool" logic
> needs some more explanation.
Because the process is always running, So stop it to reduce waste of resources.
> 19. src/backend/replication/logical/applyparallelworker.c -
> LogicalParallelApplyLoop
>
> + ApplyMessageContext = AllocSetContextCreate(ApplyContext,
> + "ApplyMessageContext",
> + ALLOCSET_DEFAULT_SIZES);
>
> Should the name of this context be "ParallelApplyMessageContext"?
I think it is okay to use "ApplyMessageContext" here just like "ApplyContext".
I will change this if more people have the same idea as you.
> 20. src/backend/replication/logical/applyparallelworker.c -
> HandleParallelApplyMessage
>
> + default:
> + {
> + elog(ERROR, "unrecognized message type received from parallel apply
> worker: %c (message length %d bytes)",
> + msgtype, msg->len);
> + }
>
> "received from" -> "received by"
>
> ~~~
>
>
> 21. src/backend/replication/logical/applyparallelworker.c -
> HandleParallelApplyMessages
>
> +/*
> + * Handle any queued protocol messages received from parallel apply workers.
> + */
> +void
> +HandleParallelApplyMessages(void)
>
> 21a.
> "received from" -> "received by"
>
> ~
>
> 21b.
> I wonder if this comment should give some credit to the function in
> parallel.c - because this seems almost a copy of all that code.
Since the message is from parallel apply worker to main apply worker, I think
"from" looks a little better.
> 27. src/backend/replication/logical/launcher.c - logicalrep_worker_detach
>
> + /*
> + * This is the leader apply worker; stop all the parallel apply workers
> + * previously started from here.
> + */
> + if (!isParallelApplyWorker(MyLogicalRepWorker))
>
> 27a.
> The comment does not match the code. If this *is* the leader apply
> worker then why do we have the condition to check that?
>
> Maybe only needs a comment update like
>
> SUGGESTION
> If this is the leader apply worker then stop all the parallel...
>
> ~
>
> 27b.
> Code seems also assuming it cannot be a tablesync worker but it is not
> checking that. I am wondering if it will be better to have yet another
> macro/inline to do isLeaderApplyWorker() that will make sure this
> really is the leader apply worker. (This review comment suggestion is
> repeated later below).
=>27a.
Improved as suggested.
=>27b.
Changed the if-statement to
`if (!am_parallel_apply_worker() && !am_tablesync_worker())`.
> 42. src/backend/replication/logical/worker.c - InitializeApplyWorker
>
> +/*
> + * Initialize the database connection, in-memory subscription and necessary
> + * config options.
> + */
>
> I still think this should mention that this is common initialization
> code for "both leader apply workers, and parallel apply workers"
I'm not sure about this. I will change this if more people have the same idea
as you.
> 44.
RE: Perform streaming logical transactions by background workers and parallel apply
On Mon, Sep 12, 2022 at 18:58 PM Kuroda, Hayato/黒田 隼人
wrote:
> Dear Hou-san,
>
> Thank you for updating the patch! Followings are comments for v28-0001.
> I will dig your patch more, but I send partially to keep the activity of the
> thread.
Thanks for your comments.
> ===
> For applyparallelworker.c
>
> 01. filename
> The word-ordering of filename seems not good
> because you defined the new worker as "parallel apply worker".
As the Amit said, keep it consistent with other file name format.
> 02. global variable
>
> ```
> +/* Parallel apply workers hash table (initialized on first use). */
> +static HTAB *ParallelApplyWorkersHash = NULL;
> +
> +/*
> + * List that stores the information of parallel apply workers that were
> + * started. Newly added worker information will be removed from the list at
> the
> + * end of the transaction when there are enough workers in the pool. Besides,
> + * exited workers will be removed from the list after being detected.
> + */
> +static List *ParallelApplyWorkersList = NIL;
> ```
>
> Could you add descriptions about difference between the list and hash table?
> IIUC the Hash stores the parallel workers that
> are assigned to transacitons, and the list stores all alive ones.
Did some modifications to the comments above ParallelApplyWorkersList.
And I think we could know the difference between these two variables by
referring to the functions parallel_apply_start_worker and
parallel_apply_free_worker.
> 03. parallel_apply_find_worker
>
> ```
> + /* Return the cached parallel apply worker if valid. */
> + if (stream_apply_worker != NULL)
> + return stream_apply_worker;
> ```
>
> This is just a question -
> Why the given xid and the assigned xid to the worker are not checked here?
> Is there chance to find wrong worker?
I think it is okay to not check the worker's xid here.
Please refer to the comments above `stream_apply_worker`.
"stream_apply_worker" will only be returned during a stream block, which means
the xid is the same as the xid in the STREAM_START message.
> 04. parallel_apply_start_worker
>
> ```
> +/*
> + * Start a parallel apply worker that will be used for the specified xid.
> + *
> + * If a parallel apply worker is not in use then re-use it, otherwise start a
> + * fresh one. Cache the worker information in ParallelApplyWorkersHash
> keyed by
> + * the specified xid.
> + */
> +void
> +parallel_apply_start_worker(TransactionId xid)
> ```
>
> "parallel_apply_start_worker" should be "start_parallel_apply_worker", I think
For code readability, similar functions are named in this format:
`parallel_apply_.*_worker`.
> 05. parallel_apply_stream_abort
>
> ```
> for (i = list_length(subxactlist) - 1; i >= 0; i--)
> {
> xid = list_nth_xid(subxactlist, i);
> if (xid == subxid)
> {
> found = true;
> break;
> }
> }
> ```
>
> Please not reuse the xid, declare and use another variable in the else block
> or
> something.
Added a temporary variable "xid_tmp" inside the for-statement.
> 06. parallel_apply_free_worker
>
> ```
> + if (napplyworkers > (max_parallel_apply_workers_per_subscription / 2))
> + {
> ```
>
> Please add a comment like: "Do we have enough workers in the pool?" or
> something.
Added the following comment according to your suggestion:
`Are there enough workers in the pool?`
> For worker.c
>
> 07. general
>
> In many lines if-else statement is used for apply_action, but I think they
> should
> rewrite as switch-case statement.
Changed.
> 08. global variable
>
> ```
> -static bool in_streamed_transaction = false;
> +bool in_streamed_transaction = false;
> ```
>
> a.
>
> It seems that in_streamed_transaction is used only in the worker.c, so we can
> change to stati variable.
>
> b.
>
> That flag is set only when an apply worker spill the transaction to the disk.
> How about "in_streamed_transaction" -> "in_spilled_transaction"?
=>8a.
Improved.
=>8b.
I am not sure if we could rename this existing variable for this. So I kept the
name.
> 09. apply_handle_stream_prepare
>
> ```
> - elog(DEBUG1, "received prepare for streamed transaction %u",
> prepare_data.xid);
> ```
>
> I think this debug message is still useful.
Since I think it is not appropriate to log the xid here, added back the
following message: `finished processing the transaction finish command`.
> 10. apply_handle_stream_stop
>
> ```
> + if (apply_action == TA_APPLY_IN_PARALLEL_WORKER)
> + {
> + pgstat_report_activity(STATE_IDLEINTRANSACTION, NULL);
> + }
> + else if (apply_action == TA_SEND_TO_PARALLEL_WORKER)
> + {
> ```
>
> The ordering of the STREAM {STOP, START} is checked only when an apply
> worker spill the transaction to the disk.
> (This is done via in_streamed_transact
RE: Perform streaming logical transactions by background workers and parallel apply
On Tues, Sep 13, 2022 at 17:49 PM Amit Kapila wrote: > Thanks for your comments. > On Fri, Sep 9, 2022 at 2:31 PM houzj.f...@fujitsu.com > wrote: > > > > On Friday, September 9, 2022 3:02 PM Peter Smith > wrote: > > > > > > > > 3. > > > > > > max_logical_replication_workers (integer) > > > Specifies maximum number of logical replication workers. This > > > includes apply leader workers, parallel apply workers, and table > > > synchronization workers. > > > Logical replication workers are taken from the pool defined by > > > max_worker_processes. > > > The default value is 4. This parameter can only be set at server > > > start. > > > > > > ~ > > > > > > I did not really understand why the default is 4. Because the default > > > tablesync workers is 2, and the default parallel workers is 2, but > > > what about accounting for the apply worker? Therefore, shouldn't > > > max_logical_replication_workers default be 5 instead of 4? > > > > The parallel apply is disabled by default, so it's not a must to increase > > this > > global default value as discussed[1] > > > > [1] https://www.postgresql.org/message- > id/CAD21AoCwaU8SqjmC7UkKWNjDg3Uz4FDGurMpis3zw5SEC%2B27jQ%40mail > .gmail.com > > > > Okay, but can we document to increase this value when the parallel > apply is enabled? Add the following sentence in the chapter [31.10. Configuration Settings]: ``` In addition, if the subscription parameter streaming is set to parallel, please increase max_logical_replication_workers according to the desired number of parallel apply workers. ``` The new patches were attached in [1]. [1] - https://www.postgresql.org/message-id/OS3PR01MB6275F145878B4A44586C46CE9E499%40OS3PR01MB6275.jpnprd01.prod.outlook.com Regards, Wang wei
RE: Perform streaming logical transactions by background workers and parallel apply
On Wed, Sep 13, 2022 at 18:26 PM Amit Kapila wrote:
>
Thanks for your comments.
> On Fri, Sep 9, 2022 at 12:32 PM Peter Smith wrote:
> >
> > 29. src/backend/replication/logical/worker.c - TransactionApplyAction
> >
> > /*
> > * What action to take for the transaction.
> > *
> > * TA_APPLY_IN_LEADER_WORKER means that we are in the leader apply
> worker and
> > * changes of the transaction are applied directly in the worker.
> > *
> > * TA_SERIALIZE_TO_FILE means that we are in leader apply worker and
> changes
> > * are written to temporary files and then applied when the final commit
> > * arrives.
> > *
> > * TA_APPLY_IN_PARALLEL_WORKER means that we are in the parallel apply
> worker
> > * and changes of the transaction are applied directly in the worker.
> > *
> > * TA_SEND_TO_PARALLEL_WORKER means that we are in the leader apply
> worker and
> > * need to send the changes to the parallel apply worker.
> > */
> > typedef enum
> > {
> > /* The action for non-streaming transactions. */
> > TA_APPLY_IN_LEADER_WORKER,
> >
> > /* Actions for streaming transactions. */
> > TA_SERIALIZE_TO_FILE,
> > TA_APPLY_IN_PARALLEL_WORKER,
> > TA_SEND_TO_PARALLEL_WORKER
> > } TransactionApplyAction;
> >
> > ~
> >
> > 29a.
> > I think if you change all those enum names slightly (e.g. like below)
> > then they can be more self-explanatory:
> >
> > TA_NOT_STREAMING_LEADER_APPLY
> > TA_STREAMING_LEADER_SERIALIZE
> > TA_STREAMING_LEADER_SEND_TO_PARALLEL
> > TA_STREAMING_PARALLEL_APPLY
> >
> > ~
> >
>
> I also think we can improve naming but adding streaming in the names
> makes them slightly difficult to read. As you have suggested, it will
> be better to add comments for streaming and non-streaming cases. How
> about naming them as below:
>
> typedef enum
> {
> TRANS_LEADER_APPLY
> TRANS_LEADER_SERIALIZE
> TRANS_LEADER_SEND_TO_PARALLEL
> TRANS_PARALLEL_APPLY
> } TransApplyAction;
I think your suggestion looks good.
Improved as suggested.
The new patches were attached in [1].
[1] -
https://www.postgresql.org/message-id/OS3PR01MB6275F145878B4A44586C46CE9E499%40OS3PR01MB6275.jpnprd01.prod.outlook.com
Regards,
Wang wei
RE: Perform streaming logical transactions by background workers and parallel apply
On Tues, Sep 13, 2022 at 20:02 PM Kuroda, Hayato/黒田 隼人
wrote:
> Dear Hou-san,
>
> > I will dig your patch more, but I send partially to keep the activity of
> > the thread.
>
> More minor comments about v28.
Thanks for your comments.
> ===
> About 0002
>
> For 015_stream.pl
>
> 14. check_parallel_log
>
> ```
> +# Check the log that the streamed transaction was completed successfully
> +# reported by parallel apply worker.
> +sub check_parallel_log
> +{
> + my ($node_subscriber, $offset, $is_parallel)= @_;
> + my $parallel_message = 'finished processing the transaction finish
> command';
> +
> + if ($is_parallel)
> + {
> + $node_subscriber->wait_for_log(qr/$parallel_message/,
> $offset);
> + }
> +}
> ```
>
> I think check_parallel_log() should be called only when streaming =
> 'parallel' and
> if-statement is not needed
I wanted to make the function test_streaming look simpler, so I put the
checking of the streaming option inside the function check_parallel_log.
> For 016_stream_subxact.pl
>
> 15. test_streaming
>
> ```
> + INSERT INTO test_tab SELECT i, md5(i::text) FROM generate_series(
> 3,
> 500) s(i);
> ```
>
> "3" should be "3".
Improved.
> About 0003
>
> For applyparallelworker.c
>
> 16. parallel_apply_relation_check()
>
> ```
> + if (rel->parallel_apply_safe == PARALLEL_APPLY_SAFETY_UNKNOWN)
> + logicalrep_rel_mark_parallel_apply(rel);
> ```
>
> I was not clear when logicalrep_rel_mark_parallel_apply() is called here.
> IIUC parallel_apply_relation_check() is called when parallel apply worker
> handles changes,
> but before that relation is opened via logicalrep_rel_open() and
> parallel_apply_safe is set here.
> If it guards some protocol violation, we may use Assert().
Compared with the flag "localrelvalid", we also need to additionally reset the
flag "safety" when function and type are changed (see function
logicalrep_relmap_init). So I think for these two cases, we just need to reset
the flag "safety" to avoid rebuilding too much cache (see function
logicalrep_relmap_reset_parallel_cb).
> For create_subscription.sgml
>
> 17.
> The restriction about foreign key does not seem to be documented.
I removed the check for the foreign key.
Since foreign key does not take effect in the subscriber's apply worker by
default, it seems that foreign key does not hit this ERROR frequently.
If we set foreign key related trigger to "REPLICA", then, I think this flag
will be set to "unsafety" when checking non-immutable function uesd by trigger.
BTW, I only document this reason in the commit message and keep the foreign key
related tests.
> ===
> About 0004
>
> For 015_stream.pl
>
> 18. check_parallel_log
>
> I heard that the removal has been reverted, but in the patch
> check_parallel_log() is removed again... :-(
Yes, I removed it.
I think this will make the test unstable. Because after applying patch
0004, we could not sure whether the transaction is completed in a parallel
apply worker. If any unexpected error occurs, the test will fail because the
log cannot be found, even if the transaction completed successfully.
> ===
> About throughout
>
> I checked the test coverage via `make coverage`. About appluparallelworker.c
> and worker.c, both function coverage is 100%, and
> line coverages are 86.2 % and 94.5 %. Generally it's good.
> But I read the report and following parts seems not tested.
>
> In parallel_apply_start_worker():
>
> ```
> if (tmp_winfo->error_mq_handle == NULL)
> {
> /*
>* Release the worker information and try next one if
> the parallel
>* apply worker exited cleanly.
>*/
> ParallelApplyWorkersList =
> foreach_delete_current(ParallelApplyWorkersList, lc);
> shm_mq_detach(tmp_winfo->mq_handle);
> dsm_detach(tmp_winfo->dsm_seg);
> pfree(tmp_winfo);
> }
> ```
>
> In HandleParallelApplyMessage():
>
> ```
> case 'X': /* Terminate, indicating
> clean exit */
> {
> shm_mq_detach(winfo->error_mq_handle);
> winfo->error_mq_handle = NULL;
> break;
> }
> ```
>
> Does it mean that we do not test the termination of parallel apply worker? If
> so I
> think it should be tested.
Since this is an unexpected situation that cannot be reproduced 100%, we did
not add tests related to this part of the code to improve coverage.
The new patches were attached in [1].
[1] -
https://www.postgresql.org/message-id/OS3PR01MB6275F145878B4A44586C46CE9E499%40OS3PR01MB6275.jpnprd01.prod.outlook.com
Regards,
Wang wei
Re: START_REPLICATION SLOT causing a crash in an assert build
On Tue, Sep 13, 2022 at 10:07:50PM -0500, Jaime Casanova wrote: > On Tue, Sep 13, 2022 at 06:48:45PM +0900, Kyotaro Horiguchi wrote: > > > > Another measure would be to add the region to wipe-out on reset to > > PgStat_KindInfo, but it seems too much.. (attached) > > > > This patch solves the problem, i didn't like the other solution because > as you say it partly nullify the protection of the assertion. > I talked too fast, while it solves the immediate problem the patch as is causes other crashes. -- Jaime Casanova Director de Servicios Profesionales SystemGuards - Consultores de PostgreSQL
Re: ICU for global collation
At Wed, 14 Sep 2022 17:19:34 +0300, Marina Polyakova wrote in > Hello! > > I was surprised that it is allowed to create clusters/databases where > the default ICU collations do not actually work due to unsupported > encodings: > > $ initdb --encoding SQL_ASCII --locale-provider icu --icu-locale en-US > -D data && > pg_ctl -D data -l logfile start && > psql -c "SELECT 'a' < 'b'" template1 > ... > waiting for server to start done > server started > ERROR: encoding "SQL_ASCII" not supported by ICU Indeed. If I did the following, the direction of the patch looks fine to me. If I executed initdb as follows, I would be told to specify --icu-locale option. > $ initdb --encoding sql-ascii --locale-provider icu hoge > ... > initdb: error: ICU locale must be specified However, when I reran the command, it complains about incompatible encoding this time. I think it's more user-friendly to check for the encoding compatibility before the check for missing --icu-locale option. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
