Re: [PoC] Improve dead tuple storage for lazy vacuum
On Tue, Jan 23, 2024 at 12:58 PM Masahiko Sawada wrote: > > On Mon, Jan 22, 2024 at 5:18 PM John Naylor wrote: > > > > On Mon, Jan 22, 2024 at 2:24 PM Masahiko Sawada > > wrote: > > > > > > For the next version patch, I'll work on this idea and try to clean up > > > locking stuff both in tidstore and radix tree. Or if you're already > > > working on some of them, please let me know. I'll review it. > > > > Okay go ahead, sounds good. I plan to look at the tests since they > > haven't been looked at in a while. > > I've attached the latest patch set. Here are updates from v54 patch: > > 0005 - Expose radix tree lock functions and remove all locks taken > internally in radixtree.h. > 0008 - Remove tidstore's control object. > 0009 - Add tidstore lock functions. > 0011 - Add VacDeadItemsInfo to store "max_bytes" and "num_items" > separate from TidStore. Also make lazy vacuum and parallel vacuum use > it. John pointed out offlist the tarball includes only the patches up to 0009. I've attached the correct one. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com v55-ART.tar.gz Description: GNU Zip compressed data
Re: Opportunistically pruning page before update
On Tue, Jan 23, 2024 at 7:18 AM James Coleman wrote: > > On Mon, Jan 22, 2024 at 8:21 PM James Coleman wrote: > > > > See rebased patch attached. > > I just realized I left a change in during the rebase that wasn't necessary. > > v4 attached. I have noticed that you are performing the opportunistic pruning after we decided that the updated tuple can not fit in the current page and then we are performing the pruning on the new target page. Why don't we first perform the pruning on the existing page of the tuple itself? Or this is already being done before this patch? I could not find such existing pruning so got this question because such pruning can convert many non-hot updates to the HOT update right? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Multiple startup messages over the same connection
On 22/01/2024 21:58, Vladimir Churyukin wrote: A question about protocol design - would it be possible to extend the protocol, so it can handle multiple startup / authentication messages over a single connection? Are there any serious obstacles? (possible issues with re-initialization of backends, I guess?) If that is possible, it could improve one important edge case - where you have to talk to multiple databases on a single host currently, you need to open a separate connection to each of them. In some cases (multitenancy for example), you may have thousands of databases on a host, which leads to inefficient connection utilization on clients (on the db side too). A lot of other RDBMSes don't have this limitation. The protocol and the startup message are the least of your problems. Yeah, it would be nice if you could switch between databases, but the assumption that one backend operates on one database is pretty deeply ingrained in the code. -- Heikki Linnakangas Neon (https://neon.tech)
Fix some errdetail's message format
We recently made corrections to the capitalization of DETAIL messages.
For example;
- GUC_check_errdetail("invalid list syntax in parameter %s",
+ GUC_check_errdetail("Invalid list syntax in parameter %s",
But still it is missing the period at the end.
There are several patterns to this issue, but this time, I have only
fixed the ones that are simple and obvious as follows:
a. GUC_check_errdetail("LITERAL"), errdetail("LITERAL") without a period.
b. psprintf()'ed string that is passed to errdetail_internal()
I didn't touch the following patterns:
c. errdetail_internal("%s")
d. errdetail("Whatever: %s")
e. errdetail("%s versus %s") and alikes
f. errdetail_internal("%s", pchomp(PQerrorMessage()))
g. complex message compilation
The attached patch contains the following fix:
- GUC_check_errdetail("timestamp out of range:
\"%s\"", str);
+ GUC_check_errdetail("Timestamp out of range:
\"%s\".", str);
But I'm not quite confident about whether capitalizing the type name
here is correct.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/contrib/cube/cubescan.l b/contrib/cube/cubescan.l
index a30fbfc311..b45dc08f0c 100644
--- a/contrib/cube/cubescan.l
+++ b/contrib/cube/cubescan.l
@@ -82,7 +82,7 @@ cube_yyerror(NDBOX **result, Size scanbuflen,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("invalid input syntax for cube"),
/* translator: %s is typically "syntax error" */
- errdetail("%s at end of input", message)));
+ errdetail("%s at end of input.", message)));
}
else
{
@@ -90,7 +90,7 @@ cube_yyerror(NDBOX **result, Size scanbuflen,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("invalid input syntax for cube"),
/* translator: first %s is typically "syntax error" */
- errdetail("%s at or near \"%s\"", message, yytext)));
+ errdetail("%s at or near \"%s\".", message, yytext)));
}
}
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 19a362526d..2720e91c14 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -2633,7 +2633,7 @@ dblink_security_check(PGconn *conn, remoteConn *rconn, const char *connstr)
ereport(ERROR,
(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
errmsg("password or GSSAPI delegated credentials required"),
- errdetail("Non-superusers may only connect using credentials they provide, eg: password in connection string or delegated GSSAPI credentials"),
+ errdetail("Non-superusers may only connect using credentials they provide, eg: password in connection string or delegated GSSAPI credentials."),
errhint("Ensure provided credentials match target server's authentication method.")));
}
diff --git a/contrib/seg/segscan.l b/contrib/seg/segscan.l
index 4ad529eccc..f5a72c2496 100644
--- a/contrib/seg/segscan.l
+++ b/contrib/seg/segscan.l
@@ -79,7 +79,7 @@ seg_yyerror(SEG *result, struct Node *escontext, const char *message)
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("bad seg representation"),
/* translator: %s is typically "syntax error" */
- errdetail("%s at end of input", message)));
+ errdetail("%s at end of input.", message)));
}
else
{
@@ -87,7 +87,7 @@ seg_yyerror(SEG *result, struct Node *escontext, const char *message)
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("bad seg representation"),
/* translator: first %s is typically "syntax error" */
- errdetail("%s at or near \"%s\"", message, yytext)));
+ errdetail("%s at or near \"%s\".", message, yytext)));
}
}
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 1b48d7171a..2d90bbfee8 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -4882,7 +4882,7 @@ check_recovery_target_time(char **newval, void **extra, GucSource source)
if (tm2timestamp(tm, fsec, , ) != 0)
{
-GUC_check_errdetail("timestamp out of range: \"%s\"", str);
+GUC_check_errdetail("Timestamp out of range: \"%s\".", str);
return false;
}
}
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 226f85d0e3..564f79dab9 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -2946,7 +2946,7 @@ AlterExtensionNamespace(const char *extensionName, const char *newschema, Oid *o
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("extension \"%s\" does not support SET SCHEMA",
NameStr(extForm->extname)),
- errdetail("%s is not in the extension's schema \"%s\"",
+ errdetail("%s is not in the extension's schema \"%s\".",
getObjectDescription(, false),
get_namespace_name(oldNspOid;
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2a56a4357c..b469d56ef0 100644
---
Re: Incremental View Maintenance, take 2
On Mon, 22 Jan 2024 13:51:08 +1100 Peter Smith wrote: > 2024-01 Commitfest. > > Hi, This patch has a CF status of "Needs Review" [1], but it seems > like there was some CFbot test failure last time it was run [2]. > Please have a look and post an updated version if necessary. Thank you for pointing out it. The CFbot failure is caused by a post [1] not by my patch-set, but regardless of it, I will heck if we need rebase and send the new version if necessary soon. [1] https://www.postgresql.org/message-id/CACJufxEoCCJE1vntJp1SWjen8vBUa3vZLgL%3DswPwar4zim976g%40mail.gmail.com Regards, Yugo Nagata > == > [1] https://commitfest.postgresql.org/46/4337/ > [2] https://cirrus-ci.com/task/6607979311529984 > > Kind Regards, > Peter Smith. -- Yugo NAGATA
Re: Removing const-false IS NULL quals and redundant IS NOT NULL quals
On Tue, Jan 23, 2024 at 1:11 PM David Rowley wrote: > I went over this again. I did a little more work adjusting comments > and pushed it. > > Thanks for all your assistance with this, Richard. Thanks for pushing! This is really great. Thanks Richard
Re: Network failure may prevent promotion
On Tue, Jan 23, 2024 at 1:23 PM Kyotaro Horiguchi
wrote:
>
> At Mon, 22 Jan 2024 13:29:10 -0800, Andres Freund wrote
> in
> > Hi,
> >
> > On 2024-01-19 12:28:05 +0900, Michael Paquier wrote:
> > > On Thu, Jan 18, 2024 at 03:42:28PM +0200, Heikki Linnakangas wrote:
> > > > Given that commit 728f86fec6 that introduced this issue was not strictly
> > > > required, perhaps we should just revert it for v16.
> > >
> > > Is there a point in keeping 728f86fec6 as well on HEAD? That does not
> > > strike me as wise to keep that in the tree for now. If it needs to be
> > > reworked, looking at this problem from scratch would be a safer
> > > approach.
> >
> > IDK, I think we'll introduce this type of bug over and over if we don't fix
> > it
> > properly.
>
> Just to clarify my position, I thought that 728f86fec6 was heading the
> right direction. Considering the current approach to signal handling
> in walreceiver, I believed that it would be better to further
> generalize in this direction rather than reverting. That's why I
> proposed that patch.
Regarding the patch, here are the review comments.
+/*
+ * Is current process a wal receiver?
+ */
+bool
+IsWalReceiver(void)
+{
+ return WalRcv != NULL;
+}
This looks wrong because WalRcv can be non-NULL in processes other
than walreceiver.
- pqsignal(SIGTERM, SignalHandlerForShutdownRequest); /* request shutdown */
+ pqsignal(SIGTERM, WalRcvShutdownSignalHandler); /* request shutdown */
Can't we just use die(), instead?
Regards,
--
Fujii Masao
Reordering DISTINCT keys to match input path's pathkeys
Similar to what we did to GROUP BY keys in 0452b461bc, I think we can do the same to DISTINCT keys, i.e. reordering DISTINCT keys to match input path's pathkeys, which can help reduce cost by avoiding unnecessary re-sort, or allowing us to use incremental-sort to save efforts. For instance, create table t (a int, b int); create index on t (a, b); explain (costs off) select distinct b, a from t limit 10; QUERY PLAN -- Limit -> Unique -> Index Only Scan using t_a_b_idx on t (3 rows) Please note that the parser has ensured that the DISTINCT pathkeys matches the order of ORDER BY clauses. So there is no need to do this part again. In principle, we can perform such reordering for DISTINCT ON too, but we need to make sure that the resulting pathkeys matches initial ORDER BY keys, which seems not trivial. So it doesn't seem worth the effort. Attached is a patch for this optimization. Any thoughts? Thanks Richard v1-0001-Reordering-DISTINCT-keys-to-match-input-path-s-pathkeys.patch Description: Binary data
Re: Shared detoast Datum proposal
Hi, Peter Smith writes: > 2024-01 Commitfest. > > Hi, This patch has a CF status of "Needs Review" [1], but it seems > there were CFbot test failures last time it was run [2]. Please have a > look and post an updated version if necessary. > > == > [1] https://commitfest.postgresql.org/46/4759/ > [2] > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/4759 v5 attached, it should fix the above issue. This version also introduce a data struct called bitset, which has a similar APIs like bitmapset but have the ability to reset all bits without recycle its allocated memory, this is important for this feature. commit 44754fb03accb0dec9710a962a334ee73eba3c49 (HEAD -> shared_detoast_value_v2) Author: yizhi.fzh Date: Tue Jan 23 13:38:34 2024 +0800 shared detoast feature. commit 14a6eafef9ff4926b8b877d694de476657deee8a Author: yizhi.fzh Date: Mon Jan 22 15:48:33 2024 +0800 Introduce a Bitset data struct. While Bitmapset is designed for variable-length of bits, Bitset is designed for fixed-length of bits, the fixed length must be specified at the bitset_init stage and keep unchanged at the whole lifespan. Because of this, some operations on Bitset is simpler than Bitmapset. The bitset_clear unsets all the bits but kept the allocated memory, this capacity is impossible for bit Bitmapset for some solid reasons and this is the main reason to add this data struct. Also for performance aspect, the functions for Bitset removed some unlikely checks, instead with some Asserts. [1] https://postgr.es/m/CAApHDvpdp9LyAoMXvS7iCX-t3VonQM3fTWCmhconEvORrQ%2BZYA%40mail.gmail.com [2] https://postgr.es/m/875xzqxbv5.fsf%40163.com I didn't write a good commit message for commit 2, the people who is interested with this can see the first message in this thread for explaination. I think anyone whose customer uses lots of jsonb probably can get benefits from this. the precondition is the toast value should be accessed 1+ times, including the jsonb_out function. I think this would be not rare to happen. -- Best Regards Andy Fan
Re: Test slots invalidations in 035_standby_logical_decoding.pl only if dead rows are removed
On Mon, Jan 22, 2024 at 09:07:45AM +, Bertrand Drouvot wrote: > On Mon, Jan 22, 2024 at 03:54:44PM +0900, Michael Paquier wrote: >> Also, wouldn't it be better to document in the test why >> txid_current_snapshot() is chosen? Contrary to txid_current(), it >> does not generate a Transaction/COMMIT to make the test more >> predictible, something you have mentioned upthread, and implied in the >> test. > > Good point, added more comments in v8 (but not mentioning txid_current() as > after giving more thought about it then I think it was not the right approach > in > any case). Fine by me. >> - INSERT INTO flush_wal DEFAULT VALUES; -- see create table flush_wal >> >> This removes two INSERTs on flush_wal and refactors the code to do the >> INSERT in wait_until_vacuum_can_remove(), using a SQL comment to >> document a reference about the reason why an INSERT is used. Couldn't >> you just use a normal comment here? > > Agree, done in v8. I've rewritten some of that, and applied the patch down to v16. Let's see how this stabilizes things, but that's likely going to take some time as it depends on skink's mood. >> I need to review what you have more thoroughly, but is it OK to assume >> that both of you are happy with the latest version of the patch in >> terms of stability gained? That's still not the full picture, still a >> good step towards that. > > Yeah, I can clearly see how this patch helps from a theoritical perspective > but > rely on Alexander's testing to see how it actually stabilizes the test. Anyway, that's not the end of it. What should we do for snapshot snapshot records coming from the bgwriter? The slower the system, the higher the odds of hitting a conflict with such records, even if the horizon check should help. -- Michael signature.asc Description: PGP signature
Re: Synchronizing slots from primary to standby
On Mon, Jan 22, 2024 at 8:42 PM Masahiko Sawada wrote: > > On Mon, Jan 22, 2024 at 9:26 PM Amit Kapila wrote: > > > > > > > > Yes. IIUC the slotsync worker uses libpqwalreceiver to establish a > > > non-replication connection and to execute SQL query. But neither of > > > them are relevant with replication. > > > > > > > But we are already using libpqwalreceiver to execute SQL queries via > > tablesync worker. > > IIUC tablesync workers do both SQL queries and replication commands. I > think the slotsync worker is the first background process who does > only SQL queries in a non-replication command ( using > libpqwalreceiver). > Yes, I agree but till now we didn't saw any problem with the same. -- With Regards, Amit Kapila.
Re: Emitting JSON to file using COPY TO
On Fri, Jan 19, 2024 at 4:10 PM Masahiko Sawada wrote:
>
> If I'm not missing, copyto_json.007.diff is the latest patch but it
> needs to be rebased to the current HEAD. Here are random comments:
>
please check the latest version.
> if (opts_out->json_mode)
> + {
> + if (is_from)
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +errmsg("cannot use JSON mode in COPY FROM")));
> + }
> + else if (opts_out->force_array)
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +errmsg("COPY FORCE_ARRAY requires JSON mode")));
>
> I think that flatting these two condition make the code more readable:
I make it two condition check
> if (opts_out->json_mode && is_from)
> ereport(ERROR, ...);
>
> if (!opts_out->json_mode && opts_out->force_array)
> ereport(ERROR, ...);
>
> Also these checks can be moved close to other checks at the end of
> ProcessCopyOptions().
>
Yes. I did it, please check it.
> @@ -3395,6 +3395,10 @@ copy_opt_item:
> {
> $$ = makeDefElem("format", (Node *) makeString("csv"),
> @1);
> }
> + | JSON
> + {
> + $$ = makeDefElem("format", (Node *) makeString("json"),
> @1);
> + }
> | HEADER_P
> {
> $$ = makeDefElem("header", (Node *) makeBoolean(true),
> @1);
> @@ -3427,6 +3431,10 @@ copy_opt_item:
> {
> $$ = makeDefElem("encoding", (Node *) makeString($2), @1);
> }
> + | FORCE ARRAY
> + {
> + $$ = makeDefElem("force_array", (Node *)
> makeBoolean(true), @1);
> + }
> ;
>
> I believe we don't need to support new options in old-style syntax.
>
> ---
> @@ -3469,6 +3477,10 @@ copy_generic_opt_elem:
> {
> $$ = makeDefElem($1, $2, @1);
> }
> + | FORMAT_LA copy_generic_opt_arg
> + {
> + $$ = makeDefElem("format", $2, @1);
> + }
> ;
>
> I think it's not necessary. "format" option is already handled in
> copy_generic_opt_elem.
>
test it, I found out this part is necessary.
because a query with WITH like `copy (select 1) to stdout with
(format json, force_array false); ` will fail.
> ---
> +/* need delimiter to start next json array element */
> +static bool json_row_delim_needed = false;
>
> I think it's cleaner to include json_row_delim_needed into CopyToStateData.
yes. I agree. So I did it.
> ---
> Splitting the patch into two patches: add json format and add
> force_array option would make reviews easy.
>
done. one patch for json format, another one for force_array option.
I also made the following cases fail.
copy copytest to stdout (format csv, force_array false);
ERROR: specify COPY FORCE_ARRAY is only allowed in JSON mode.
If copy to table then call table_scan_getnextslot no need to worry
about the Tupdesc.
however if we copy a query output as format json, we may need to consider it.
cstate->queryDesc->tupDesc is the output of Tupdesc, we can rely on this.
for copy a query result to json, I memcpy( cstate->queryDesc->tupDesc)
to the the slot's slot->tts_tupleDescriptor
so composite_to_json can use cstate->queryDesc->tupDesc to do the work.
I guess this will make it more bullet-proof.
From 214ad534d13730cba13008798c3d70f8b363436f Mon Sep 17 00:00:00 2001
From: jian he
Date: Tue, 23 Jan 2024 12:26:43 +0800
Subject: [PATCH v8 2/2] Add force_array for COPY TO json fomrat.
make add open brackets and close for the whole output.
separate each json row with comma after the first row.
---
doc/src/sgml/ref/copy.sgml | 14 ++
src/backend/commands/copy.c| 17 +
src/backend/commands/copyto.c | 30 ++
src/backend/parser/gram.y | 4
src/include/commands/copy.h| 1 +
src/test/regress/expected/copy.out | 24
src/test/regress/sql/copy.sql | 10 ++
7 files changed, 100 insertions(+)
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index ccd90b61..d19332ac 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -43,6 +43,7 @@ COPY { table_name [ ( column_name [, ...] ) | * }
FORCE_NOT_NULL { ( column_name [, ...] ) | * }
FORCE_NULL { ( column_name [, ...] ) | * }
+FORCE_ARRAY [ boolean ]
ON_ERROR 'error_action'
ENCODING 'encoding_name'
@@ -379,6 +380,19 @@ COPY { table_name [ (
+
+FORCE_ARRAY
+
+
+ Force output of square brackets as array decorations at the beginning
+ and end of output, and commas between the rows. It is allowed only in
+ COPY TO, and only when using
+ JSON format. The default is
+ false.
+
+
+
+
ON_ERROR
Re: Removing const-false IS NULL quals and redundant IS NOT NULL quals
On Tue, 23 Jan 2024 at 00:11, David Rowley wrote: > I've attached v11 which updates the expected results in some newly > added regression tests. I went over this again. I did a little more work adjusting comments and pushed it. Thanks for all your assistance with this, Richard. David
Re: core dumps in auto_prewarm, tests succeed
Hello Andres, 22.01.2024 23:41, Andres Freund wrote: Hi, I noticed that I was getting core dumps while executing the tests, without the tests failing. Backtraces are vriations of this: ... ISTM that we shouldn't basically silently overlook shutdowns due to crashes in the tests. How to not do so is unfortunately not immediately obvious to me... FWIW, I encountered this behavior as well (with pg_stat): https://www.postgresql.org/message-id/18158-88f667028dbc7...@postgresql.org and proposed a way to detect such shutdowns for a discussion: https://www.postgresql.org/message-id/flat/290b9ae3-98a2-0896-a957-18d3b60b6260%40gmail.com where Shveta referenced a previous thread started by Tom Lane: https://www.postgresql.org/message-id/flat/2366244.1651681...@sss.pgh.pa.us What do you think about leaving postmaster.pid on disk in case of an abnormal shutdown? Best regards, Alexander
Unnecessary smgropen in {heapam_relation,index}_copy_data?
Hi, hackers
I find heapam_relation_copy_data() and index_copy_data() have the following
code:
dstrel = smgropen(*newrlocator, rel->rd_backend);
...
RelationCreateStorage(*newrlocator, rel->rd_rel->relpersistence, true);
The smgropen() is also called by RelationCreateStorage(), why should we call
smgropen() explicitly here?
I try to remove the smgropen(), and all tests passed.
>From 88a6670dcff67036014fd6b31062bcab5daed30e Mon Sep 17 00:00:00 2001
From: japinli
Date: Tue, 23 Jan 2024 12:34:18 +0800
Subject: [PATCH v1 1/1] Remove unnecessary smgropen
---
src/backend/access/heap/heapam_handler.c | 4 +---
src/backend/commands/tablecmds.c | 4 +---
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 5a17112c91..547fdef26a 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -637,8 +637,6 @@ heapam_relation_copy_data(Relation rel, const RelFileLocator *newrlocator)
{
SMgrRelation dstrel;
- dstrel = smgropen(*newrlocator, rel->rd_backend);
-
/*
* Since we copy the file directly without looking at the shared buffers,
* we'd better first flush out any pages of the source relation that are
@@ -654,7 +652,7 @@ heapam_relation_copy_data(Relation rel, const RelFileLocator *newrlocator)
* NOTE: any conflict in relfilenumber value will be caught in
* RelationCreateStorage().
*/
- RelationCreateStorage(*newrlocator, rel->rd_rel->relpersistence, true);
+ dstrel = RelationCreateStorage(*newrlocator, rel->rd_rel->relpersistence, true);
/* copy main fork */
RelationCopyStorage(RelationGetSmgr(rel), dstrel, MAIN_FORKNUM,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index da705ae468..5ca6277ee0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -14787,8 +14787,6 @@ index_copy_data(Relation rel, RelFileLocator newrlocator)
{
SMgrRelation dstrel;
- dstrel = smgropen(newrlocator, rel->rd_backend);
-
/*
* Since we copy the file directly without looking at the shared buffers,
* we'd better first flush out any pages of the source relation that are
@@ -14804,7 +14802,7 @@ index_copy_data(Relation rel, RelFileLocator newrlocator)
* NOTE: any conflict in relfilenumber value will be caught in
* RelationCreateStorage().
*/
- RelationCreateStorage(newrlocator, rel->rd_rel->relpersistence, true);
+ dstrel = RelationCreateStorage(newrlocator, rel->rd_rel->relpersistence, true);
/* copy main fork */
RelationCopyStorage(RelationGetSmgr(rel), dstrel, MAIN_FORKNUM,
base-commit: 80bece312c4b957ea5a93db84be1d1776f0e5e67
--
2.34.1
Re: Report planning memory in EXPLAIN ANALYZE
On Thu, Jan 18, 2024 at 5:28 PM Alvaro Herrera wrote:
>
> On 2024-Jan-18, Ashutosh Bapat wrote:
>
> > The EXPLAIN output produces something like below
> >explain_filter
> > -
> >Seq Scan on int8_tbl i8 (cost=N.N..N.N rows=N width=N)
> > Planning:
> > Memory: used=N bytes, allocated=N bytes
> > (3 rows)
> >
> > but function explain_filter(), defined in explain.sql, removes line
> > containing Planning: and we end up with output
> >explain_filter
> > -
> >Seq Scan on int8_tbl i8 (cost=N.N..N.N rows=N width=N)
> > Memory: used=N bytes, allocated=N bytes
> > (2 rows)
> >
> > Hence this weird difference.
>
> Ah, okay, that makes sense. (I actually think this is a bit insane, and
> I would hope that we can revert commit eabba4a3eb71 eventually, but I
> don't think that needs to hold up your proposed patch.)
>
Thanks. Attached is rebased and squashed patch.
--
Best Wishes,
Ashutosh Bapat
From 1c4f33ce05fdfdbeab6e7f35d064aaa97373536b Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat
Date: Wed, 12 Jul 2023 14:34:14 +0530
Subject: [PATCH] EXPLAIN reports memory consumed for planning a query
The memory consumed is reported as "Memory" property under "Planning"
section in EXPLAIN output when option MEMORY is specified.
A separate memory context is allocated for measuring memory consumption
to eliminate any effect of previous activity on the calculation. The
memory context statistics is noted before and after calling
pg_plan_query() from ExplainOneQuery(). The difference in the two
statistics is used to calculate the memory consumption.
We report two values "used" and "allocated". The difference between
memory statistics counters totalspace and freespace gives the memory
that remains palloc'ed at the end of planning. It is reported as memory
"used". This does not account for chunks of memory that were palloc'ed
but subsequently pfree'ed. But such memory remains allocated in the
memory context is given by the totalspace counter. The value of this
counter is reported as memory "allocated".
Author: Ashutosh Bapat
Reviewed-by: David Rowley, Andrey Lepikhov, Jian He, Andy Fan
Reviewed-by: Alvaro Herrera
Discussion: https://www.postgresql.org/message-id/CAExHW5sZA=5lj_zppro-w09ck8z9p7eayaqq3ks9gdfhrxe...@mail.gmail.com
---
contrib/auto_explain/auto_explain.c | 2 +
doc/src/sgml/ref/explain.sgml | 14 +++
src/backend/commands/explain.c| 124 ++
src/backend/commands/prepare.c| 29 +-
src/backend/utils/mmgr/mcxt.c | 13 +++
src/include/commands/explain.h| 4 +-
src/include/utils/memutils.h | 2 +
src/test/regress/expected/explain.out | 68 ++
src/test/regress/sql/explain.sql | 6 ++
9 files changed, 245 insertions(+), 17 deletions(-)
diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c
index c7aacd7812..dcb5d6c9e5 100644
--- a/contrib/auto_explain/auto_explain.c
+++ b/contrib/auto_explain/auto_explain.c
@@ -396,6 +396,8 @@ explain_ExecutorEnd(QueryDesc *queryDesc)
es->wal = (es->analyze && auto_explain_log_wal);
es->timing = (es->analyze && auto_explain_log_timing);
es->summary = es->analyze;
+ /* No support for MEMORY option */
+ /* es->memory = false; */
es->format = auto_explain_log_format;
es->settings = auto_explain_log_settings;
diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml
index 5ba6486da1..26809d68d5 100644
--- a/doc/src/sgml/ref/explain.sgml
+++ b/doc/src/sgml/ref/explain.sgml
@@ -44,6 +44,7 @@ EXPLAIN [ ( option [, ...] ) ] boolean ]
TIMING [ boolean ]
SUMMARY [ boolean ]
+MEMORY [ boolean ]
FORMAT { TEXT | XML | JSON | YAML }
@@ -250,6 +251,19 @@ ROLLBACK;
+
+MEMORY
+
+
+ Include information on memory consumption by the query planning phase.
+ Specifically, include the precise amount of storage used by planner
+ in-memory structures, as well as total memory considering allocation
+ overhead.
+ The parameter defaults to FALSE.
+
+
+
+
FORMAT
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 843472e6dd..e912f7dae0 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -119,9 +119,12 @@ static void show_instrumentation_count(const char *qlabel, int which,
static void show_foreignscan_info(ForeignScanState *fsstate, ExplainState *es);
static void show_eval_params(Bitmapset *bms_params, ExplainState *es);
static const char *explain_get_index_name(Oid indexId);
-static void show_buffer_usage(ExplainState *es, const BufferUsage *usage,
+static bool show_buffer_usage(ExplainState *es, const BufferUsage *usage,
bool planning);
static void
Re: Permute underscore separated components of columns before fuzzy matching
Thank you! I wasn't aware of the filter per person. It was quite simple
integrate a web scraper into my custom push system.
Regarding the patch: I ran the 2.1.1 version of pg_bsd_indent now. I
hope that suffices. I removed the matrix declaration to make it C90
complaint. I attached the result.
Regards
Arne
On 2024-01-22 19:22, Tom Lane wrote:
Arne Roland writes:
Thank you for bringing that to my attention. Is there a way to subscribe
to cf-bot failures?
I don't know of any push notification support in cfbot, but you
can bookmark the page with your own active patches, and check it
periodically:
http://commitfest.cputube.org/arne-roland.html
(For others, click on your own name in the main cfbot page's entry for
one of your patches to find out how it spelled your name for this
purpose.)
regards, tom lanediff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 34a0ec5901..69abac7c92 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -579,37 +579,18 @@ GetCTEForRTE(ParseState *pstate, RangeTblEntry *rte, int rtelevelsup)
return NULL;/* keep compiler quiet */
}
-/*
- * updateFuzzyAttrMatchState
- * Using Levenshtein distance, consider if column is best fuzzy match.
- */
static void
-updateFuzzyAttrMatchState(int fuzzy_rte_penalty,
- FuzzyAttrMatchState *fuzzystate, RangeTblEntry *rte,
- const char *actual, const char *match, int attnum)
+updateFuzzyAttrMatchStateSingleString(int fuzzy_rte_penalty,
+ FuzzyAttrMatchState *fuzzystate, RangeTblEntry *rte,
+ const char *actual, const char *match, int attnum, int matchlen)
{
- int columndistance;
- int matchlen;
-
- /* Bail before computing the Levenshtein distance if there's no hope. */
- if (fuzzy_rte_penalty > fuzzystate->distance)
- return;
-
- /*
- * Outright reject dropped columns, which can appear here with apparent
- * empty actual names, per remarks within scanRTEForColumn().
- */
- if (actual[0] == '\0')
- return;
-
/* Use Levenshtein to compute match distance. */
- matchlen = strlen(match);
- columndistance =
- varstr_levenshtein_less_equal(actual, strlen(actual), match, matchlen,
- 1, 1, 1,
- fuzzystate->distance + 1
- - fuzzy_rte_penalty,
- true);
+ int columndistance =
+ varstr_levenshtein_less_equal(actual, strlen(actual), match, matchlen,
+ 1, 1, 1,
+ fuzzystate->distance + 1
+ - fuzzy_rte_penalty,
+ true);
/*
* If more than half the characters are different, don't treat it as a
@@ -667,6 +648,207 @@ updateFuzzyAttrMatchState(int fuzzy_rte_penalty,
}
}
+static void
+putUnderscores(char *string, int start, int underscore_amount, int len)
+{
+ for (int i = 0; start + i < len && i < underscore_amount; i++)
+ string[start + i] = '_';
+}
+
+/*
+ * updateFuzzyAttrMatchState
+ * Using Levenshtein distance, consider if column is best fuzzy match.
+ */
+static void
+updateFuzzyAttrMatchState(int fuzzy_rte_penalty,
+ FuzzyAttrMatchState *fuzzystate, RangeTblEntry *rte,
+ const char *actual, const char *match, int attnum)
+{
+ /* Memory segment to store the current permutation of the match string. */
+ char *tmp_match;
+ int matchlen = strlen(match);
+
+ /*
+ * We keep track how many permutations we have already processed, to avoid
+ * long runtimes.
+ */
+ int underscore_permutations_count = 0;
+
+ /*
+ * The location the underscore we currently process within the match
+ * string.
+ */
+ int underscore_current = 1;
+
+ /* Variables to track the amount of underscores delimiting sections */
+ int underscore_amount = 1;
+ int underscore_second_amount = 1;
+
+ /*
+ * The entries of the permutation matrix tell us, where we should copy the
+ * tree segments to. The zeroth dimension iterates over the permutations,
+ * while the first dimension iterates over the three segments are permuted
+ * to. Considering the string A_B_C the three segments are:
+ * - A: before the initial underscore sections
+ * - B: between the underscore sections
+ * - C: after the later underscore sections
+ *
+ * Please note that the _ in above example are placeholders for
+ * underscore_amount underscores, which might be more than one.
+ */
+ int permutation_matrix[3][3];
+
+ /* Bail before computing the Levenshtein distance if there's no hope. */
+ if (fuzzy_rte_penalty > fuzzystate->distance)
+ return;
+
+ /*
+ * Outright reject dropped columns, which can appear here with apparent
+ * empty actual names, per remarks within scanRTEForColumn().
+ */
+ if (actual[0] == '\0')
+ return;
+
+ updateFuzzyAttrMatchStateSingleString(fuzzy_rte_penalty, fuzzystate, rte, actual, match, attnum, matchlen);
+
+ /*
+ * We don't want to permute zero length strings, so check whether the
+ * string starts with an underscore.
+ */
+ if (match[0] == '_')
+ {
+ while (underscore_current <
Re: psql: Allow editing query results with \gedit
po 22. 1. 2024 v 23:54 odesílatel Christoph Berg napsal:
> Re: David G. Johnston
> > Building off the other comments, I'd suggest trying to get rid of the
> > intermediate JSOn format and also just focus on a single row at any given
> > time.
>
> We need *some* machine-readable format. It doesn't have to be JSON,
> but JSON is actually pretty nice to read - and if values are too long,
> or there are too many values, switch to extended mode:
>
> select * from messages \gedit (expanded)
>
> [{
> "id": "1",
> "language": "en",
> "message": "This is a very long test text with little actual meaning."
> },{
> "id": "2",
> "language": "en",
> "message": "Another one, a bit shorter."
> }]
>
> I tweaked the indentation in the psql JSON output patch specifically
> to make it readable.
>
> Restricting to a single line might make sense if it helps editing, but
> I don't think it does.
>
> > For an update the first argument to the metacommand could be the unique
> key
> > value present in the previous result. The resultant UPDATE would just
> put
> > that into the where clause and every other column in the result would be
> a
> > SET clause column with the thing being set the current value, ready to be
> > edited.
>
> Hmm, then you would still have to cut-and-paste the PK value. If that
> that's a multi-column non-numeric key, you are basically back to the
> original problem.
>
>
> Re: Tom Lane
> > Yeah, that's something that was also bothering me, but I failed to
> > put my finger on it. "Here's some JSON, edit it, and don't forget
> > to keep the quoting correct" does not strike me as a user-friendly
> > way to adjust data content. A spreadsheet-like display where you
> > can change data within cells seems like a far better API, although
> > I don't want to write that either.
>
> Right. I wouldn't want a form editing system in there either. But
> perhaps this middle ground of using a well-established format that is
> easy to generate and to parse (it's using the JSON parser from
> pgcommon) makes it fit into psql.
>
> If parsing the editor result fails, the user is asked if they want to
> re-edit with a parser error message, and if they go to the editor
> again, the cursor is placed in the line where the error is. (Also,
> what's wrong with having to strictly adhere to some syntax, we are
> talking about SQL here.)
>
> It's admittedly larger than the average \backslash command, but it
> does fit into psql's interactive usage. \crosstabview is perhaps a
> similar thing - it doesn't really fit into a simple "send query and
> display result" client, but since it solves an actual problem, it
> makes well sense to spend the extra code on it.
>
\crosstabview is read only
>
> > This kind of API would not readily support INSERT or DELETE cases, but
> > TBH I think that's better anyway --- you're adding too much ambiguity
> > in pursuit of a very secondary use-case. The stated complaint was
> > "it's too hard to build UPDATE commands", which I can sympathize with.
>
> I've been using the feature already for some time, and it's a real
> relief. In my actual use case here, I use it on my ham radio logbook:
>
> =# select start, call, qrg, name from log where cty = 'CE9' order by start;
> start │ call │ qrg │ name
> ┼┼─┼───
> 2019-03-12 20:34:00+00 │ RI1ANL │7.076253 │ ∅
> 2021-03-16 21:24:00+00 │ DP0GVN │2400.395 │ Felix
> 2022-01-15 17:19:00+00 │ DP0GVN │ 2400.01 │ Felix
> 2022-10-23 19:17:15+00 │ DP0GVN │ 2400.041597 │ ∅
> 2023-10-01 14:05:00+00 │ 8J1RL │ 28.182575 │ ∅
> 2024-01-22 21:15:15+00 │ DP1POL │ 10.138821 │ ∅
> (6 Zeilen)
>
> The primary key is (start, call).
>
> If I now want to note that the last contact with Antarctica there was
> also with Felix, I'd have to transform that into
>
> update log set name = 'Felix' where start = '2024-01-22 21:15:15+00' and
> call = 'DP1POL';
>
> \gedit is just so much easier.
>
It looks great for simple queries, but if somebody uses it like SELECT *
FROM pg_proc \gedit
I almost sure so \gedit is wrong name for this feature.
Can be nice if we are able:
a) export data set in some readable format
b) be possible to use more command in pipes
some like
select start, call, qrg, name from log where cty = 'CE9' order by start
\gpipexec(tsv) mypipe | bash update_pattern.sh > tmpfile; vi tmpfile; cat
tmpfile > mypipe
I understand your motivation well, but I don't like your proposal because
too many different things are pushed to one feature, and it is designed for
a single purpose.
UPDATE is the core feature. If we want to say INSERT and DELETE aren't
> supported, but UPDATE support can go in, that'd be fine with me.
>
> > (BTW, I wonder how much of this already exists in pgAdmin.)
>
> pgadmin seems to support it. (Most other clients don't.)
>
> Obviously, I would want to do the updating using the client I also use
> for querying.
>
> Christoph
>
Re: Network failure may prevent promotion
At Mon, 22 Jan 2024 13:29:10 -0800, Andres Freund wrote in > Hi, > > On 2024-01-19 12:28:05 +0900, Michael Paquier wrote: > > On Thu, Jan 18, 2024 at 03:42:28PM +0200, Heikki Linnakangas wrote: > > > Given that commit 728f86fec6 that introduced this issue was not strictly > > > required, perhaps we should just revert it for v16. > > > > Is there a point in keeping 728f86fec6 as well on HEAD? That does not > > strike me as wise to keep that in the tree for now. If it needs to be > > reworked, looking at this problem from scratch would be a safer > > approach. > > IDK, I think we'll introduce this type of bug over and over if we don't fix it > properly. Just to clarify my position, I thought that 728f86fec6 was heading the right direction. Considering the current approach to signal handling in walreceiver, I believed that it would be better to further generalize in this direction rather than reverting. That's why I proposed that patch. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: In-placre persistance change of a relation
At Mon, 22 Jan 2024 15:36:31 +1100, Peter Smith wrote
in
> 2024-01 Commitfest.
>
> Hi, This patch has a CF status of "Needs Review" [1], but it seems
> there was a CFbot test failure last time it was run [2]. Please have a
> look and post an updated version if necessary.
Thanks! I have added the necessary includes to the header file this
patch adds. With this change, "make headerscheck" now passes. However,
when I run "make cpluspluscheck" in my environment, it generates a
large number of errors in other areas, but I didn't find one related
to this patch.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 9a2b6fbda882587c127d3e50bccf89508837d1a5 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Mon, 15 Jan 2024 15:57:53 +0900
Subject: [PATCH v32 1/3] Export wal_sync_method related functions
Export several functions related to wal_sync_method for use in
subsequent commits. Since PG_O_DIRECT cannot be used in those commits,
the new function XLogGetSyncBit() will mask PG_O_DIRECT.
---
src/backend/access/transam/xlog.c | 73 +--
src/include/access/xlog.h | 2 +
2 files changed, 52 insertions(+), 23 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 478377c4a2..c5f51849ee 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8403,21 +8403,29 @@ assign_wal_sync_method(int new_wal_sync_method, void *extra)
}
}
+/*
+ * Exported version of get_sync_bit()
+ *
+ * Do not expose PG_O_DIRECT for uses outside xlog.c.
+ */
+int
+XLogGetSyncBit(void)
+{
+ return get_sync_bit(wal_sync_method) & ~PG_O_DIRECT;
+}
+
/*
- * Issue appropriate kind of fsync (if any) for an XLOG output file.
+ * Issue appropriate kind of fsync (if any) according to wal_sync_method.
*
- * 'fd' is a file descriptor for the XLOG file to be fsync'd.
- * 'segno' is for error reporting purposes.
+ * 'fd' is a file descriptor for the file to be fsync'd.
*/
-void
-issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
+const char *
+XLogFsyncFile(int fd)
{
- char *msg = NULL;
+ const char *msg = NULL;
instr_time start;
- Assert(tli != 0);
-
/*
* Quick exit if fsync is disabled or write() has already synced the WAL
* file.
@@ -8425,7 +8433,7 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
if (!enableFsync ||
wal_sync_method == WAL_SYNC_METHOD_OPEN ||
wal_sync_method == WAL_SYNC_METHOD_OPEN_DSYNC)
- return;
+ return NULL;
/* Measure I/O timing to sync the WAL file */
if (track_wal_io_timing)
@@ -8460,19 +8468,6 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
break;
}
- /* PANIC if failed to fsync */
- if (msg)
- {
- char xlogfname[MAXFNAMELEN];
- int save_errno = errno;
-
- XLogFileName(xlogfname, tli, segno, wal_segment_size);
- errno = save_errno;
- ereport(PANIC,
-(errcode_for_file_access(),
- errmsg(msg, xlogfname)));
- }
-
pgstat_report_wait_end();
/*
@@ -8486,7 +8481,39 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
INSTR_TIME_ACCUM_DIFF(PendingWalStats.wal_sync_time, end, start);
}
- PendingWalStats.wal_sync++;
+ if (msg != NULL)
+ PendingWalStats.wal_sync++;
+
+ return msg;
+}
+
+/*
+ * Issue appropriate kind of fsync (if any) for an XLOG output file.
+ *
+ * 'fd' is a file descriptor for the XLOG file to be fsync'd.
+ * 'segno' is for error reporting purposes.
+ */
+void
+issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
+{
+ const char *msg;
+
+ Assert(tli != 0);
+
+ msg = XLogFsyncFile(fd);
+
+ /* PANIC if failed to fsync */
+ if (msg)
+ {
+ char xlogfname[MAXFNAMELEN];
+ int save_errno = errno;
+
+ XLogFileName(xlogfname, tli, segno, wal_segment_size);
+ errno = save_errno;
+ ereport(PANIC,
+(errcode_for_file_access(),
+ errmsg(msg, xlogfname)));
+ }
}
/*
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 301c5fa11f..2a0d65b537 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -217,6 +217,8 @@ extern void xlog_redo(struct XLogReaderState *record);
extern void xlog_desc(StringInfo buf, struct XLogReaderState *record);
extern const char *xlog_identify(uint8 info);
+extern int XLogGetSyncBit(void);
+extern const char *XLogFsyncFile(int fd);
extern void issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli);
extern bool RecoveryInProgress(void);
--
2.39.3
>From c464013071dedc15b838e573ae828f150b3b60f7 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Thu, 31 Aug 2023 11:49:10 +0900
Subject: [PATCH v32 2/3] Introduce undo log implementation
This patch adds a simple implementation of UNDO log feature.
---
src/backend/access/transam/Makefile| 1 +
src/backend/access/transam/meson.build | 1 +
src/backend/access/transam/rmgr.c | 4 +-
src/backend/access/transam/simpleundolog.c | 362 +
src/backend/access/transam/twophase.c |
Re: Synchronizing slots from primary to standby
Here are some review comments for v65-0002
==
0. General - GUCs in messages
I think it would be better for the GUC names to all be quoted. It's
not a rule (yet), but OTOH it seems to be the consensus most people
want. See [1].
This might impact the following messages:
0.1
+ ereport(ERROR,
+ errmsg("could not fetch primary_slot_name \"%s\" info from the"
+" primary server: %s", PrimarySlotName, res->err));
SUGGESTION
errmsg("could not fetch primary server slot \"%s\" info from the
primary server: %s", ...)
errhint("Check if \"primary_slot_name\" is configured correctly.");
~~~
0.2
+ if (!tuplestore_gettupleslot(res->tuplestore, true, false, tupslot))
+ elog(ERROR, "failed to fetch primary_slot_name tuple");
SUGGESTION
elog(ERROR, "failed to fetch tuple for the primary server slot
specified by \"primary_slot_name\"");
~~~
0.3
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ /* translator: second %s is a GUC variable name */
+ errdetail("The primary server slot \"%s\" specified by %s is not valid.",
+ PrimarySlotName, "primary_slot_name"));
/specified by %s/specified by \"%s\"/
~~~
0.4
+ ereport(ERROR,
+ /* translator: %s is a GUC variable name */
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ errhint("%s must be defined.", "primary_slot_name"));
/%s must be defined./\"%s\" must be defined./
~~~
0.5
+ ereport(ERROR,
+ /* translator: %s is a GUC variable name */
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ errhint("%s must be enabled.", "hot_standby_feedback"));
/%s must be enabled./\"%s\" must be enabled./
~~~
0.6
+ ereport(ERROR,
+ /* translator: %s is a GUC variable name */
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ errhint("wal_level must be >= logical."));
errhint("\"wal_level\" must be >= logical."))
~~~
0.7
+ if (PrimaryConnInfo == NULL || strcmp(PrimaryConnInfo, "") == 0)
+ ereport(ERROR,
+ /* translator: %s is a GUC variable name */
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ errhint("%s must be defined.", "primary_conninfo"));
/%s must be defined./\"%s\" must be defined./
~~~
0.8
+ ereport(ERROR,
+
+ /*
+ * translator: 'dbname' is a specific option; %s is a GUC variable
+ * name
+ */
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ errhint("'dbname' must be specified in %s.", "primary_conninfo"));
/must be specified in %s./must be specified in \"%s\"./
~~~
0.9
+ ereport(LOG,
+ errmsg("skipping slot synchronization"),
+ errdetail("enable_syncslot is disabled."));
errdetail("\"enable_syncslot\" is disabled."));
==
src/backend/replication/logical/slotsync.c
1.
+/* Min and Max sleep time for slot sync worker */
+#define MIN_WORKER_NAPTIME_MS 200
+#define MAX_WORKER_NAPTIME_MS 3 /* 30s */
+
+/*
+ * Sleep time in ms between slot-sync cycles.
+ * See wait_for_slot_activity() for how we adjust this
+ */
+static long sleep_ms = MIN_WORKER_NAPTIME_MS;
These all belong together, so I think they share a combined comment like:
SUGGESTION
The sleep time (ms) between slot-sync cycles varies dynamically
(within a MIN/MAX range) according to slot activity. See
wait_for_slot_activity() for details.
~~~
2. update_and_persist_slot
+ /* First time slot update, the function must return true */
+ if(!local_slot_update(remote_slot))
+ elog(ERROR, "failed to update slot");
Missing whitespace after 'if'
~~~
3. synchronize_one_slot
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("exiting from slot synchronization because same"
+" name slot \"%s\" already exists on standby",
+remote_slot->name),
+ errdetail("A user-created slot with the same name as"
+ " failover slot already exists on the standby."));
3a.
/on standby/on the standby/
~
3b.
Now the errmsg is changed, the errdetail doesn't seem so useful. Isn't
it repeating pretty much the same information as in the errmsg?
==
src/backend/replication/walsender.c
4. GetStandbyFlushRecPtr
/*
- * Returns the latest point in WAL that has been safely flushed to disk, and
- * can be sent to the standby. This should only be called when in recovery,
- * ie. we're streaming to a cascaded standby.
+ * Returns the latest point in WAL that has been safely flushed to disk.
+ * This should only be called when in recovery.
+ *
Since it says "This should only be called when in recovery", should
there also be a check for that (e.g. RecoveryInProgress) in the added
Assert?
==
src/include/replication/walreceiver.h
5.
typedef char *(*walrcv_identify_system_fn) (WalReceiverConn *conn,
TimeLineID *primary_tli);
+/*
+ * walrcv_get_dbname_from_conninfo_fn
+ *
+ * Returns the dbid from the primary_conninfo
+ */
+typedef char *(*walrcv_get_dbname_from_conninfo_fn) (const char *conninfo);
It looks like a blank line that previously existed has been lost.
Re: Make mesage at end-of-recovery less scary.
At Wed, 17 Jan 2024 14:32:00 +0900, Michael Paquier wrote in > On Tue, Jan 16, 2024 at 02:46:02PM +0300, Aleksander Alekseev wrote: > >> For now, let me explain the basis for this patch. The fundamental > >> issue is that these warnings that always appear are, in practice, not > >> a problem in almost all cases. Some of those who encounter them for > >> the first time may feel uneasy and reach out with inquiries. On the > >> other hand, those familiar with these warnings tend to ignore them and > >> only pay attention to details when actual issues arise. Therefore, the > >> intention of this patch is to label them as "no issue" unless a > >> problem is blatantly evident, in order to prevent unnecessary concern. > > > > I agree and don't mind affecting the error message per se. > > > > However I see that the actual logic of how WAL is processed is being > > changed. If we do this, at very least it requires thorough thinking. I > > strongly suspect that the proposed code is wrong and/or not safe > > and/or less safe than it is now for the reasons named above. > > FWIW, that pretty much sums up my feeling regarding this patch, > because an error, basically any error, would hurt back very badly. > Sure, the error messages we generate now when reaching the end of WAL > can sound scary, and they are (I suspect that's not really the case > for anybody who has history doing support with PostgreSQL because a > bunch of these messages are old enough to vote, but I can understand > that anybody would freak out the first time they see that). > > However, per the recent issues we've had in this area, like > cd7f19da3468 but I'm more thinking about 6b18b3fe2c2f and > bae868caf222, I am of the opinion that the header validation, the > empty page case in XLogReaderValidatePageHeader() and the record read > changes are risky enough that I am not convinced that the gains are > worth the risks taken. > > The error stack in the WAL reader is complicated enough that making it > more complicated as the patch proposes does not sound like not a good > tradeoff to me to make the reports related to the end of WAL cleaner > for the end-user. I agree that we should do something, but the patch > does not seem like a good step towards this goal. Perhaps somebody > would be more excited about this proposal than I am, of course. Thank you both for the comments. The criticism seems valid. The approach to identifying the end-of-WAL state in this patch is quite heuristic, and its validity or safety can certainly be contested. On the other hand, if we seek perfection in this area of judgment, we may need to have the WAL format itself more robust. In any case, since the majority of the feedback on this patch seems to be negative, I am going to withdraw it if no supportive opinions emerge during this commit-fest. The attached patch addresses the errors reported by CF-bot. regards. -- Kyotaro Horiguchi NTT Open Source Software Center >From 933d10fa6c7b71e4684f5ba38e85177afaa56f58 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi Date: Tue, 7 Mar 2023 14:55:58 +0900 Subject: [PATCH v29] Make End-Of-Recovery error less scary When recovery in any type ends, we see a bit scary error message like "invalid record length" that suggests something serious is happening. Actually if recovery meets a record with length = 0, that usually means it finished applying all available WAL records. Make this message less scary as "reached end of WAL". Instead, raise the error level for other kind of WAL failure to WARNING. To make sure that the detection is correct, this patch checks if all trailing bytes in the same page are zeroed in that case. --- src/backend/access/transam/xlogreader.c | 147 ++ src/backend/access/transam/xlogrecovery.c | 96 ++ src/backend/replication/walreceiver.c | 7 +- src/bin/pg_waldump/pg_waldump.c | 13 +- src/bin/pg_waldump/t/001_basic.pl | 5 +- src/include/access/xlogreader.h | 1 + src/test/recovery/t/035_recovery.pl | 130 +++ src/test/recovery/t/039_end_of_wal.pl | 47 +-- 8 files changed, 383 insertions(+), 63 deletions(-) create mode 100644 src/test/recovery/t/035_recovery.pl diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c index 7190156f2f..94861969eb 100644 --- a/src/backend/access/transam/xlogreader.c +++ b/src/backend/access/transam/xlogreader.c @@ -48,6 +48,8 @@ static int ReadPageInternal(XLogReaderState *state, XLogRecPtr pageptr, int reqLen); static void XLogReaderInvalReadState(XLogReaderState *state); static XLogPageReadResult XLogDecodeNextRecord(XLogReaderState *state, bool nonblocking); +static bool ValidXLogRecordLength(XLogReaderState *state, XLogRecPtr RecPtr, + XLogRecord *record); static bool ValidXLogRecordHeader(XLogReaderState *state, XLogRecPtr RecPtr, XLogRecPtr PrevRecPtr, XLogRecord *record, bool
Re: Add \syncpipeline command to pgbench
On Mon, Jan 22, 2024 at 01:59:00PM +0100, Alvaro Herrera wrote: > On 2024-Jan-22, Anthonin Bonnefoy wrote: >> 0002 adds the \syncpipeline command (original patch with an additional >> test case). > > I can look into this one later, unless Michael wants to. The patch seemed rather OK at quick glance as long as there is a test to check for error path with a \syncpipeline still on the stack of metacommands to handle. Anyway, I wanted to study this one and learn a bit more about the error stuff that was happening on pgbench side. Now, if you feel strongly about it, please go ahead! -- Michael signature.asc Description: PGP signature
Re: Improve WALRead() to suck data directly from WAL buffers when possible
On Mon, 2024-01-22 at 12:12 -0800, Andres Freund wrote:
> I still think that anything that requires such checks shouldn't be
> merged. It's completely bogus to check page contents for validity
> when we
> should have metadata telling us which range of the buffers is valid
> and which
> not.
The check seems entirely unnecessary, to me. A leftover from v18?
I have attached a new patch (version "19j") to illustrate some of my
previous suggestions. I didn't spend a lot of time on it so it's not
ready for commit, but I believe my suggestions are easier to understand
in code form.
Note that, right now, it only works for XLogSendPhysical(). I believe
it's best to just make it work for 1-3 callers that we understand well,
and we can generalize later if it makes sense.
I'm still not clear on why some callers are reading XLOG_BLCKSZ
(expecting zeros at the end), and if it's OK to just change them to use
the exact byte count.
Also, if we've detected that the first requested buffer has been
evicted, is there any value in continuing the loop to see if more
recent buffers are available? For example, if the requested LSNs range
over buffers 4, 5, and 6, and 4 has already been evicted, should we try
to return LSN data from 5 and 6 at the proper offset in the dest
buffer? If so, we'd need to adjust the API so the caller knows what
parts of the dest buffer were filled in.
Regards,
Jeff Davis
From 34d89d6b869a454fd15097d8a0f0b6d3997a74da Mon Sep 17 00:00:00 2001
From: Jeff Davis
Date: Mon, 22 Jan 2024 16:21:53 -0800
Subject: [PATCH v19j 1/2] Add XLogReadFromBuffers().
Allows reading directly from WAL buffers without a lock, avoiding the
need to wait for WAL flushing and read from the filesystem.
For now, the only caller is physical replication, but we can consider
expanding it to other callers as needed.
Author: Bharath Rupireddy
---
src/backend/access/transam/xlog.c | 145 ++--
src/backend/replication/walsender.c | 14 +++
src/include/access/xlog.h | 2 +
3 files changed, 152 insertions(+), 9 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 478377c4a2..c9619139af 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -698,7 +698,7 @@ static void ReserveXLogInsertLocation(int size, XLogRecPtr *StartPos,
XLogRecPtr *EndPos, XLogRecPtr *PrevPtr);
static bool ReserveXLogSwitch(XLogRecPtr *StartPos, XLogRecPtr *EndPos,
XLogRecPtr *PrevPtr);
-static XLogRecPtr WaitXLogInsertionsToFinish(XLogRecPtr upto);
+static XLogRecPtr WaitXLogInsertionsToFinish(XLogRecPtr upto, bool emitLog);
static char *GetXLogBuffer(XLogRecPtr ptr, TimeLineID tli);
static XLogRecPtr XLogBytePosToRecPtr(uint64 bytepos);
static XLogRecPtr XLogBytePosToEndRecPtr(uint64 bytepos);
@@ -1494,7 +1494,7 @@ WALInsertLockUpdateInsertingAt(XLogRecPtr insertingAt)
* to make room for a new one, which in turn requires WALWriteLock.
*/
static XLogRecPtr
-WaitXLogInsertionsToFinish(XLogRecPtr upto)
+WaitXLogInsertionsToFinish(XLogRecPtr upto, bool emitLog)
{
uint64 bytepos;
XLogRecPtr reservedUpto;
@@ -1521,9 +1521,10 @@ WaitXLogInsertionsToFinish(XLogRecPtr upto)
*/
if (upto > reservedUpto)
{
- ereport(LOG,
-(errmsg("request to flush past end of generated WAL; request %X/%X, current position %X/%X",
- LSN_FORMAT_ARGS(upto), LSN_FORMAT_ARGS(reservedUpto;
+ if (emitLog)
+ ereport(LOG,
+ (errmsg("request to flush past end of generated WAL; request %X/%X, current position %X/%X",
+ LSN_FORMAT_ARGS(upto), LSN_FORMAT_ARGS(reservedUpto;
upto = reservedUpto;
}
@@ -1705,6 +1706,132 @@ GetXLogBuffer(XLogRecPtr ptr, TimeLineID tli)
return cachedPos + ptr % XLOG_BLCKSZ;
}
+/*
+ * Read WAL directly from WAL buffers, if available.
+ *
+ * This function reads 'count' bytes of WAL from WAL buffers into 'buf'
+ * starting at location 'startptr' and returns total bytes read.
+ *
+ * The bytes read may be fewer than requested if any of the WAL buffers in the
+ * requested range have been evicted, or if the last requested byte is beyond
+ * the Insert pointer.
+ *
+ * If reading beyond the Write pointer, this function will wait for concurent
+ * inserters to finish. Otherwise, it does not wait at all.
+ *
+ * The caller must ensure that it's reasonable to read from the WAL buffers,
+ * i.e. that the requested data is from the current timeline, that we're not
+ * in recovery, etc.
+ */
+Size
+XLogReadFromBuffers(char *buf, XLogRecPtr startptr, Size count)
+{
+ XLogRecPtr ptr = startptr;
+ XLogRecPtr upto = startptr + count;
+ Size nbytes = count;
+ Size ntotal = 0;
+ char *dst = buf;
+
+ Assert(!RecoveryInProgress());
+ Assert(!XLogRecPtrIsInvalid(startptr));
+
+ /*
+ * Caller requested very recent WAL data. Wait for any in-progress WAL
+ * insertions to WAL buffers to finish.
+ *
+ * Most callers will have already updated LogwrtResult when
Re: Make mesage at end-of-recovery less scary.
At Mon, 22 Jan 2024 16:09:28 +1100, Peter Smith wrote in > 2024-01 Commitfest. > > Hi, This patch has a CF status of "Needs Review" [1], but it seems > there were CFbot test failures last time it was run [2]. Please have a > look and post an updated version if necessary. > > == > [1] https://commitfest.postgresql.org/46/2490/ > [2] > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/2490 Thanks for noticing of that. Will repost a new version. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: [PoC] Improve dead tuple storage for lazy vacuum
On Mon, Jan 22, 2024 at 5:18 PM John Naylor wrote: > > On Mon, Jan 22, 2024 at 2:24 PM Masahiko Sawada wrote: > > > > For the next version patch, I'll work on this idea and try to clean up > > locking stuff both in tidstore and radix tree. Or if you're already > > working on some of them, please let me know. I'll review it. > > Okay go ahead, sounds good. I plan to look at the tests since they > haven't been looked at in a while. I've attached the latest patch set. Here are updates from v54 patch: 0005 - Expose radix tree lock functions and remove all locks taken internally in radixtree.h. 0008 - Remove tidstore's control object. 0009 - Add tidstore lock functions. 0011 - Add VacDeadItemsInfo to store "max_bytes" and "num_items" separate from TidStore. Also make lazy vacuum and parallel vacuum use it. The new patches probably need to be polished but the VacDeadItemInfo idea looks good to me. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com v55-ART.tar.gz Description: GNU Zip compressed data
Re: Add \syncpipeline command to pgbench
On Mon, Jan 22, 2024 at 05:53:13PM +0100, Alvaro Herrera wrote:
> I hope this is OK. I debated a half a dozen alternatives ("with open
> pipeline", "without closing pipeline", "with unclosed pipeline" (???),
> "leaving pipeline open") and decided this was the least ugly.
That looks OK to me. Thanks for looking at that!
--
Michael
signature.asc
Description: PGP signature
Re: Adding facility for injection points (or probe points?) for more advanced tests
On Tue, Jan 23, 2024 at 12:08:17PM +0900, Michael Paquier wrote:
> That was on my TODO list of things to tackle and propose, but perhaps
> there is no point in waiting more so I've applied your patch.
Slightly off topic and while I don't forget about it.. Please find
attached a copy of the patch posted around [1] to be able to define
injection points with input arguments, so as it is possible to execute
callbacks with values coming from the code path where the point is
attached.
For example, a backend could use this kind of macro to have a callback
attached to this point use some runtime value:
INJECTION_POINT_1ARG("InjectionPointBoo", _value);
[1]: https://www.postgresql.org/message-id/Za8TLyD9HIjzFlhJ%40paquier.xyz
--
Michael
From 1ebcd3c50c9db555b3d2078db931e8c8bdb87472 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 23 Jan 2024 12:30:52 +0900
Subject: [PATCH] Extend injection points with optional runtime arguments
This extends injections points with a 1-argument flavor, so as it
becomes possible for callbacks to perform runtime checks.
---
src/include/utils/injection_point.h | 7 +-
src/backend/utils/misc/injection_point.c | 75 +++
.../expected/injection_points.out | 25 +++
.../injection_points--1.0.sql | 10 +++
.../injection_points/injection_points.c | 31 +++-
.../injection_points/sql/injection_points.sql | 7 ++
doc/src/sgml/xfunc.sgml | 10 ++-
7 files changed, 146 insertions(+), 19 deletions(-)
diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h
index 55524b568f..8e1527bb4a 100644
--- a/src/include/utils/injection_point.h
+++ b/src/include/utils/injection_point.h
@@ -16,22 +16,27 @@
*/
#ifdef USE_INJECTION_POINTS
#define INJECTION_POINT(name) InjectionPointRun(name)
+#define INJECTION_POINT_1ARG(name, arg1) InjectionPointRun1Arg(name, arg1)
#else
#define INJECTION_POINT(name) ((void) name)
+#define INJECTION_POINT_1ARG(name) ((void) name, (void) arg1)
#endif
/*
* Typedef for callback function launched by an injection point.
*/
typedef void (*InjectionPointCallback) (const char *name);
+typedef void (*InjectionPointCallback1Arg) (const char *name, void *arg1);
extern Size InjectionPointShmemSize(void);
extern void InjectionPointShmemInit(void);
extern void InjectionPointAttach(const char *name,
const char *library,
- const char *function);
+ const char *function,
+ int num_args);
extern void InjectionPointRun(const char *name);
+extern void InjectionPointRun1Arg(const char *name, void *arg1);
extern void InjectionPointDetach(const char *name);
#endif /* INJECTION_POINT_H */
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
index 0cf4d51cac..468085e963 100644
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -49,6 +49,7 @@ typedef struct InjectionPointEntry
char name[INJ_NAME_MAXLEN]; /* hash key */
char library[INJ_LIB_MAXLEN]; /* library */
char function[INJ_FUNC_MAXLEN]; /* function */
+ int num_args; /* number of arguments */
} InjectionPointEntry;
#define INJECTION_POINT_HASH_INIT_SIZE 16
@@ -61,7 +62,12 @@ typedef struct InjectionPointEntry
typedef struct InjectionPointCacheEntry
{
char name[INJ_NAME_MAXLEN];
- InjectionPointCallback callback;
+ int num_args;
+ union
+ {
+ InjectionPointCallback callback;
+ InjectionPointCallback1Arg callback_1arg;
+ };
} InjectionPointCacheEntry;
static HTAB *InjectionPointCache = NULL;
@@ -73,7 +79,8 @@ static HTAB *InjectionPointCache = NULL;
*/
static void
injection_point_cache_add(const char *name,
- InjectionPointCallback callback)
+ void *callback,
+ int num_args)
{
InjectionPointCacheEntry *entry;
bool found;
@@ -99,6 +106,13 @@ injection_point_cache_add(const char *name,
Assert(!found);
strlcpy(entry->name, name, sizeof(entry->name));
entry->callback = callback;
+ entry->num_args = num_args;
+ if (num_args == 0)
+ entry->callback = (InjectionPointCallback) callback;
+ else if (num_args == 1)
+ entry->callback_1arg = (InjectionPointCallback1Arg) callback;
+ else
+ elog(ERROR, "unsupported number of arguments"); /* not reachable */
}
/*
@@ -123,7 +137,7 @@ injection_point_cache_remove(const char *name)
*
* Retrieve an injection point from the local cache, if any.
*/
-static InjectionPointCallback
+static InjectionPointCacheEntry *
injection_point_cache_get(const char *name)
{
bool found;
@@ -137,7 +151,7 @@ injection_point_cache_get(const char *name)
hash_search(InjectionPointCache, name, HASH_FIND, );
if (found)
- return entry->callback;
+ return entry;
return NULL;
}
@@ -186,7 +200,8 @@ InjectionPointShmemInit(void)
void
InjectionPointAttach(const char *name,
const char *library,
- const char *function)
+ const char *function,
+
Re: Add support for data change delta tables
On 1/13/24 12:41, PavelTurk wrote: Hello all, Hi Pavel! Currently PostgreSQL doesn't support data change delta tables. For example, it doesn't support this type of query: SELECT * FROM NEW TABLE ( INSERT INTO phone_book VALUES ( 'Peter Doe', '555-2323' ) ) AS t Correct. We do not yet support that. PostgreSQL has RETURNING that provides only a subset of this functionality. I think that because of the way postgres is designed, it will only ever provide a subset of that functionality anyway. Other people know more of the internals than I do, but I don't think we can easily distinguish between NEW TABLE and FINAL TABLE. Unfortunately, your example does not show how postgres is inadequate. For example, INSERT INTO t1 (c1) SELECT c2 FROM OLD TABLE ( DELETE FROM t2 WHERE ... ) AS t can be written as WITH old_table (c2) AS ( DELETE FROM t2 WHERE ... RETURNING c2 ) INSERT INTO t1 (c1) TABLE old_table So I suggest to add support for data change delta tables. Because this feature is more powerful and it is included in the SQL Standard. It is indeed included in the SQL Standard, but is it really more powerful? Consider this example which is currently not implemented but could be, and compare it to the standard where such a query could not be possible at all: UPDATE t SET a = ... WHERE ... RETURNING OLD.a, NEW.a, FINAL.a All this being said, I would love to see data change delta tables implemented per-spec in PostgreSQL; in addition to improving the RETURNING clause. I believe I have heard that we can't just do a syntactic transformation because the trigger execution order is not the same, but I would have to research that. -- Vik Fearing
Re: Adding facility for injection points (or probe points?) for more advanced tests
On Mon, Jan 22, 2024 at 09:02:48PM +0200, Heikki Linnakangas wrote: > On 22/01/2024 18:08, Heikki Linnakangas wrote: >> I wrote the attached patch to enable injection points in the Cirrus CI >> config, to run the injection tests I wrote for a GIN bug today [1]. But >> that led to a crash in the asan-enabled build [2]. I didn't investigate >> it yet. > > Pushed a fix for the crash. That's embarrassing. Thanks for the quick fix. > What do you think of enabling this in the Cirrus CI config? That was on my TODO list of things to tackle and propose, but perhaps there is no point in waiting more so I've applied your patch. -- Michael signature.asc Description: PGP signature
Re: Support "Right Semi Join" plan shapes
On Mon, 22 Jan 2024 at 11:27, wenhui qiu wrote: > > Hi vignesh CI saw this path has been passed > (https://cirrus-ci.com/build/6109321080078336),can we push it? If you have found no comments from your review and testing, let's mark it as "ready for committer". Regards, Vignesh
Re: introduce dynamic shared memory registry
On Mon, Jan 22, 2024 at 05:00:48PM +0530, Bharath Rupireddy wrote: > On Mon, Jan 22, 2024 at 3:43 AM Nathan Bossart > wrote: >> Oops. I've attached an attempt at fixing this. I took the opportunity to >> clean up the surrounding code a bit. > > The code looks cleaner and readable with the patch. All the call sites > are taking care of dsm_attach returning NULL value. So, the attached > patch looks good to me. Committed. Thanks for the report and the reviews. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Things I don't like about \du's "Attributes" column
On Sun, Jan 21, 2024 at 2:35 PM Pavel Luzanov wrote: > List of roles > Role name | Attributes | Password? | Valid until | Connection > limit > ---+-+---++-- > admin | INHERIT | no|| > -1 > alice | SUPERUSER +| yes | infinity | > 5 > > I think I'm in the minority on believing that these describe outputs should not be beholden to internal implementation details. But seeing a -1 in the limit column is just jarring to my sensibilities. I suggest displaying blank (not null, \pset should not influence this) if the connection limit is "no limit", only showing positive numbers when there is meaningful exceptional information for the user to absorb. David J.
Re: Things I don't like about \du's "Attributes" column
On Mon, Jan 22, 2024 at 6:26 PM Tom Lane wrote: > I wrote: > > I think expecting the pg_roles view to change for this is problematic. > > You can't have that in the back branches, so with this patch psql > > will show something different against a pre-17 server than later > > versions. At best, that's going to be confusing. > > Actually, even more to the point: while this doesn't expose the > contents of a role's password, it does expose whether the role > *has* a password to every user in the installation. I doubt > that that's okay from a security standpoint. It'd need debate > at the least. > > Makes sense, more reason to put it within its own patch. At present it seems like a createrole permissioned user is unable to determine whether a given role has a password or not even in the case when that role would be allowed to alter a role they've created to set or remove said password. Keeping with the changes made in v16 it does seem worthwhile modifying pg_roles to be sensitive to the role querying the view having both createrole and admin membership on the role being displayed. With now three possible outcomes: NULL if no password is in use, * if a password is in use and the user has the ability to alter role, or (alt. N/A). David J.
Re: make dist using git archive
On Tue, Jan 23, 2024 at 2:36 AM Peter Eisentraut wrote: > > On 22.01.24 13:10, Junwang Zhao wrote: > > I played this with meson build on macOS, the packages are generated > > in source root but not build root, I'm sure if this is by design but I think > > polluting *working directory* is not good. > > Yes, it's not good, but I couldn't find a way to make it work. > > This is part of the complications with meson I referred to. The > @BUILD_ROOT@ placeholder in custom_target() is apparently always a > relative path, but it doesn't know that git -C changes the current > directory. > > > Another thing I'd like to point out is, should we also introduce *git > > commit* > > or maybe *git tag* to package name, something like: > > > > git archive --format tar.gz --prefix postgresql-17devel/ HEAD -o > > postgresql-17devel-`git rev-parse --short HEAD`.tar.gz > > git archive --format tar.gz --prefix postgresql-17devel/ HEAD -o > > postgresql-`git describe --tags`.tar.gz > > I'm not sure why we would need it built-in. It can be done by hand, of > course. If this is only used by the release phase, one can do this by hand. *commit id/tag* in package name can be used to identify the git source, which might be useful for cooperation between QA and dev team, but surely there are better ways for this, so I do not have a strong opinion here. > -- Regards Junwang Zhao
Re: Opportunistically pruning page before update
On Mon, Jan 22, 2024 at 8:21 PM James Coleman wrote: > > See rebased patch attached. I just realized I left a change in during the rebase that wasn't necessary. v4 attached. Regards, James Coleman v4-0002-Opportunistically-prune-to-avoid-building-a-new-p.patch Description: Binary data v4-0001-Allow-getting-lock-before-calling-heap_page_prune.patch Description: Binary data
Re: Make documentation builds reproducible
On Tue, Jan 23, 2024 at 12:13 PM Tom Lane wrote: > > Peter Smith writes: > > I usually the HTML documentation locally using command: > > make STYLE=website html > > This has been working forever, but seems to have broken due to commit > > [1] having an undeclared variable. > > Interestingly, that still works fine for me, on RHEL8 with > > docbook-dtds-1.0-69.el8.noarch > docbook-style-xsl-1.79.2-9.el8.noarch > docbook-style-dsssl-1.79-25.el8.noarch > > What docbook version are you using? > [postgres@CentOS7-x64 sgml]$ sudo yum list installed | grep docbook docbook-dtds.noarch 1.0-60.el7 @anaconda docbook-style-dsssl.noarch1.79-18.el7@base docbook-style-xsl.noarch 1.78.1-3.el7 @anaconda == Kind Regards, Peter Smith. Fujitsu Australia
Re: Things I don't like about \du's "Attributes" column
I wrote: > I think expecting the pg_roles view to change for this is problematic. > You can't have that in the back branches, so with this patch psql > will show something different against a pre-17 server than later > versions. At best, that's going to be confusing. Actually, even more to the point: while this doesn't expose the contents of a role's password, it does expose whether the role *has* a password to every user in the installation. I doubt that that's okay from a security standpoint. It'd need debate at the least. regards, tom lane
Re: Opportunistically pruning page before update
On Sun, Jan 21, 2024 at 9:58 PM Peter Smith wrote: > > 2024-01 Commitfest. > > Hi, This patch has a CF status of "Needs Review" [1], but it seems > like there was some CFbot test failure last time it was run [2]. > Please have a look and post an updated version if necessary. > > == > [1] https://commitfest.postgresql.org/46/4384// > [2] > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/4384 See rebased patch attached. Thanks, James Coleman v3-0001-Allow-getting-lock-before-calling-heap_page_prune.patch Description: Binary data v3-0002-Opportunistically-prune-to-avoid-building-a-new-p.patch Description: Binary data
Re: Things I don't like about \du's "Attributes" column
Pavel Luzanov writes: > Another approach based on early suggestions. I think expecting the pg_roles view to change for this is problematic. You can't have that in the back branches, so with this patch psql will show something different against a pre-17 server than later versions. At best, that's going to be confusing. Can you get the same result without changing pg_roles? regards, tom lane
Re: Make documentation builds reproducible
Peter Smith writes: > I usually the HTML documentation locally using command: > make STYLE=website html > This has been working forever, but seems to have broken due to commit > [1] having an undeclared variable. Interestingly, that still works fine for me, on RHEL8 with docbook-dtds-1.0-69.el8.noarch docbook-style-xsl-1.79.2-9.el8.noarch docbook-style-dsssl-1.79-25.el8.noarch What docbook version are you using? regards, tom lane
Re: Make documentation builds reproducible
Hi,
I usually the HTML documentation locally using command:
make STYLE=website html
~
This has been working forever, but seems to have broken due to commit
[1] having an undeclared variable.
e.g.
[postgres@CentOS7-x64 sgml]$ make STYLE=website html
{ \
echo ""; \
echo ""; \
} > version.sgml
'/usr/bin/perl' ./mk_feature_tables.pl YES
../../../src/backend/catalog/sql_feature_packages.txt
../../../src/backend/catalog/sql_features.txt >
features-supported.sgml
'/usr/bin/perl' ./mk_feature_tables.pl NO
../../../src/backend/catalog/sql_feature_packages.txt
../../../src/backend/catalog/sql_features.txt >
features-unsupported.sgml
'/usr/bin/perl' ./generate-errcodes-table.pl
../../../src/backend/utils/errcodes.txt > errcodes-table.sgml
'/usr/bin/perl' ./generate-keywords-table.pl . > keywords-table.sgml
'/usr/bin/perl' ./generate-targets-meson.pl targets-meson.txt
generate-targets-meson.pl > targets-meson.sgml
'/usr/bin/perl'
../../../src/backend/utils/activity/generate-wait_event_types.pl
--docs ../../../src/backend/utils/activity/wait_event_names.txt
/usr/bin/xmllint --nonet --path . --path . --output postgres-full.xml
--noent --valid postgres.sgml
/usr/bin/xsltproc --nonet --path . --path . --stringparam pg.version
'17devel' --param website.stylesheet 1 stylesheet.xsl
postgres-full.xml
runtime error: file stylesheet-html-common.xsl line 452 element if
Variable 'autolink.index.see' has not been declared.
make: *** [html-stamp] Error 10
==
[1]
https://github.com/postgres/postgres/commit/b0f0a9432d0b6f53634a96715f2666f6d4ea25a1
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: Add last_commit_lsn to pg_stat_database
On Sun, Jan 21, 2024 at 10:26 PM Peter Smith wrote: > > 2024-01 Commitfest. > > Hi, This patch has a CF status of "Needs Review", but it seems like > there was some CFbot test failure last time it was run [1]. Please > have a look and post an updated version if necessary. > > == > [1] > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/4355 > > Kind Regards, > Peter Smith. Updated patch attached, Thanks, James Coleman v3-0001-Add-last-commit-s-LSN-to-pg_stat_database.patch Description: Binary data
Re: Built-in CTYPE provider
On Mon, 2024-01-22 at 19:49 +0100, Peter Eisentraut wrote:
> >
> I don't get this argument. Of course, people care about sorting and
> sort order. Whether you consider this part of Unicode or adjacent to
> it, people still want it.
You said that my proposal sends a message that we somehow don't care
about Unicode, and I strongly disagree. The built-in provider I'm
proposing does implement Unicode semantics.
Surely a database that offers UCS_BASIC (a SQL spec feature) isn't
sending a message that it doesn't care about Unicode, and neither is my
proposal.
> >
> > * ICU offers COLLATE UNICODE, locale tailoring, case-insensitive
> > matching, and customization with rules. It's the solution for
> > everything from "slightly more advanced" to "very advanced".
>
> I am astonished by this. In your world, do users not want their text
> data sorted? Do they not care what the sort order is?
I obviously care about Unicode and collation. I've put a lot of effort
recently into contributions in this area, and I wouldn't have done that
if I thought users didn't care. You've made much greater contributions
and I thank you for that.
The logical conclusion of your line of argument would be that libc's
"C.UTF-8" locale and UCS_BASIC simply should not exist. But they do
exist, and for good reason.
One of those good reasons is that only *human* users care about the
human-friendliness of sort order. If Postgres is just feeding the
results to another system -- or an application layer that re-sorts the
data anyway -- then stability, performance, and interoperability matter
more than human-friendliness. (Though Unicode character semantics are
still useful even when the data is not going directly to a human.)
> You consider UCA
> sort order an "advanced" feature?
I said "slightly more advanced" compared with "basic". "Advanced" can
be taken in either a positive way ("more useful") or a negative way
("complex"). I'm sorry for the misunderstanding, but my point was this:
* The builtin provider is for people who are fine with code point order
and no tailoring, but want Unicode character semantics, collation
stability, and performance.
* ICU is the right solution for anyone who wants human-friendly
collation or tailoring, and is willing to put up with some collation
stability risk and lower collation performance.
Both have their place and the user is free to mix and match as needed,
thanks to the COLLATE clause for columns and queries.
Regards,
Jeff Davis
Re: Things I don't like about \du's "Attributes" column
On Sun, Jan 21, 2024 at 2:35 PM Pavel Luzanov wrote: > Another approach based on early suggestions. > > The Attributes column includes only the enabled logical attributes. > Regardless of whether the attribute is enabled by default or not. > > > The attribute names correspond to the keywords of the CREATE ROLE command. > The attributes are listed in the same order as in the documentation. > (I think that the LOGIN attribute should be moved to the first place, > both in the documentation and in the command.) > > I'd just flip INHERIT and LOGIN > The "Connection limit" and "Valid until" attributes are placed in separate > columns. > The "Password?" column has been added. > > Sample output. > > Patch v3: > =# \du > List of roles > Role name |Attributes > | Password? | Valid until | Connection limit > ---+---+---++-- > admin | INHERIT > | no|| -1 > alice | SUPERUSER LOGIN > | yes | infinity |5 > bob | CREATEDB INHERIT LOGIN REPLICATION BYPASSRLS > | no| 2022-01-01 00:00:00+03 | -1 > charlie | CREATEROLE INHERIT LOGIN > | yes ||0 > postgres | SUPERUSER CREATEDB CREATEROLE INHERIT LOGIN REPLICATION > BYPASSRLS | no|| -1 > (5 rows) > > > Small modification with newline separator for Attributes column: > > Patch v3 with newlines: > =# \du > List of roles > Role name | Attributes | Password? | Valid until | Connection > limit > ---+-+---++-- > postgres | SUPERUSER +| no|| > -1 >| CREATEDB +| || >| CREATEROLE +| || >| INHERIT+| || >| LOGIN +| || >| REPLICATION+| || >| BYPASSRLS | || > (5 rows) > > I'm strongly in favor of using mixed-case for the attributes. The SQL Command itself doesn't care about capitalization and it is much easier on the eyes. I'm also strongly in favor of newlines, as can be seen by the default bootstrap superuser entry putting everything on one line eats up 65 characters. List of roles Role name | Attributes | Password? | Valid until | Connection limit | Description ---+-+---+-+--+- davidj| Superuser +| no| | -1 | | CreateDB +| | | | | CreateRole +| | | | | Inherit+| | | | | Login +| | | | | Replication+| | | | | BypassRLS | | | | (1 row) As noted by Peter this patch didn't update the two affected expected output files. psql.out and, due to the system view change, rules.out. That particular change requires a documentation update to the pg_roles system view page. I'd suggest pulling out this system view change into its own patch. I will take another pass later when I get some more time. I want to re-review some of the older messages. But the tweaks I show and breaking out the view changes in to a separate patch both appeal to me right now. David J.
Re: psql: Allow editing query results with \gedit
Re: David G. Johnston
> Building off the other comments, I'd suggest trying to get rid of the
> intermediate JSOn format and also just focus on a single row at any given
> time.
We need *some* machine-readable format. It doesn't have to be JSON,
but JSON is actually pretty nice to read - and if values are too long,
or there are too many values, switch to extended mode:
select * from messages \gedit (expanded)
[{
"id": "1",
"language": "en",
"message": "This is a very long test text with little actual meaning."
},{
"id": "2",
"language": "en",
"message": "Another one, a bit shorter."
}]
I tweaked the indentation in the psql JSON output patch specifically
to make it readable.
Restricting to a single line might make sense if it helps editing, but
I don't think it does.
> For an update the first argument to the metacommand could be the unique key
> value present in the previous result. The resultant UPDATE would just put
> that into the where clause and every other column in the result would be a
> SET clause column with the thing being set the current value, ready to be
> edited.
Hmm, then you would still have to cut-and-paste the PK value. If that
that's a multi-column non-numeric key, you are basically back to the
original problem.
Re: Tom Lane
> Yeah, that's something that was also bothering me, but I failed to
> put my finger on it. "Here's some JSON, edit it, and don't forget
> to keep the quoting correct" does not strike me as a user-friendly
> way to adjust data content. A spreadsheet-like display where you
> can change data within cells seems like a far better API, although
> I don't want to write that either.
Right. I wouldn't want a form editing system in there either. But
perhaps this middle ground of using a well-established format that is
easy to generate and to parse (it's using the JSON parser from
pgcommon) makes it fit into psql.
If parsing the editor result fails, the user is asked if they want to
re-edit with a parser error message, and if they go to the editor
again, the cursor is placed in the line where the error is. (Also,
what's wrong with having to strictly adhere to some syntax, we are
talking about SQL here.)
It's admittedly larger than the average \backslash command, but it
does fit into psql's interactive usage. \crosstabview is perhaps a
similar thing - it doesn't really fit into a simple "send query and
display result" client, but since it solves an actual problem, it
makes well sense to spend the extra code on it.
> This kind of API would not readily support INSERT or DELETE cases, but
> TBH I think that's better anyway --- you're adding too much ambiguity
> in pursuit of a very secondary use-case. The stated complaint was
> "it's too hard to build UPDATE commands", which I can sympathize with.
I've been using the feature already for some time, and it's a real
relief. In my actual use case here, I use it on my ham radio logbook:
=# select start, call, qrg, name from log where cty = 'CE9' order by start;
start │ call │ qrg │ name
┼┼─┼───
2019-03-12 20:34:00+00 │ RI1ANL │7.076253 │ ∅
2021-03-16 21:24:00+00 │ DP0GVN │2400.395 │ Felix
2022-01-15 17:19:00+00 │ DP0GVN │ 2400.01 │ Felix
2022-10-23 19:17:15+00 │ DP0GVN │ 2400.041597 │ ∅
2023-10-01 14:05:00+00 │ 8J1RL │ 28.182575 │ ∅
2024-01-22 21:15:15+00 │ DP1POL │ 10.138821 │ ∅
(6 Zeilen)
The primary key is (start, call).
If I now want to note that the last contact with Antarctica there was
also with Felix, I'd have to transform that into
update log set name = 'Felix' where start = '2024-01-22 21:15:15+00' and call =
'DP1POL';
\gedit is just so much easier.
UPDATE is the core feature. If we want to say INSERT and DELETE aren't
supported, but UPDATE support can go in, that'd be fine with me.
> (BTW, I wonder how much of this already exists in pgAdmin.)
pgadmin seems to support it. (Most other clients don't.)
Obviously, I would want to do the updating using the client I also use
for querying.
Christoph
Re: core dumps in auto_prewarm, tests succeed
On Mon, Jan 22, 2024 at 03:38:15PM -0600, Nathan Bossart wrote: > On Mon, Jan 22, 2024 at 01:24:54PM -0800, Andres Freund wrote: >> On 2024-01-22 15:19:36 -0600, Nathan Bossart wrote: >>> I think this is because the autoprewarm state was moved to a DSM segment, >>> and DSM segments are detached before the on_shmem_exit callbacks are called >>> during process exit. Moving apw_detach_shmem to the before_shmem_exit list >>> seems to resolve the crashes. >> >> That seems plausible. Would still be nice to have at least this test ensure >> that the shutdown code works. Perhaps just a check of the control file after >> shutdown, ensuring that the state is "shutdowned" vs crashed? > > I'll give that a try. I'll also expand the comment above the > before_shmem_exit() call. Here is a patch. This might be a topic for another thread, but I do wonder whether we could put a generic pg_controldata check in node->stop that would at least make sure that the state is some sort of expected shut-down state. My first thought is that it could be a tad expensive, but... maybe it wouldn't be too bad. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com >From b339a84802b76d42f4863e52e6d91ab28873e00a Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Mon, 22 Jan 2024 16:22:57 -0600 Subject: [PATCH v1 1/1] fix autoprewarm core dump --- contrib/pg_prewarm/autoprewarm.c | 10 -- contrib/pg_prewarm/t/001_basic.pl | 6 ++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c index 9ea6c2252a..06ee21d496 100644 --- a/contrib/pg_prewarm/autoprewarm.c +++ b/contrib/pg_prewarm/autoprewarm.c @@ -164,8 +164,14 @@ autoprewarm_main(Datum main_arg) if (apw_init_shmem()) first_time = false; - /* Set on-detach hook so that our PID will be cleared on exit. */ - on_shmem_exit(apw_detach_shmem, 0); + /* + * Set on-detach hook so that our PID will be cleared on exit. + * + * NB: Autoprewarm's state is stored in a DSM segment, and DSM segments + * are detached before calling the on_shmem_exit callbacks, so we must put + * apw_detach_shmem in the before_shmem_exit callback list. + */ + before_shmem_exit(apw_detach_shmem, 0); /* * Store our PID in the shared memory area --- unless there's already diff --git a/contrib/pg_prewarm/t/001_basic.pl b/contrib/pg_prewarm/t/001_basic.pl index bcd23a6914..825d3448ee 100644 --- a/contrib/pg_prewarm/t/001_basic.pl +++ b/contrib/pg_prewarm/t/001_basic.pl @@ -55,4 +55,10 @@ $node->wait_for_log( $node->stop; +# control file should indicate normal shut down +command_like( + [ 'pg_controldata', $node->data_dir() ], + qr/Database cluster state:\s*shut down/, + 'cluster shut down normally'); + done_testing(); -- 2.25.1
Re: core dumps in auto_prewarm, tests succeed
On Mon, Jan 22, 2024 at 01:24:54PM -0800, Andres Freund wrote: > On 2024-01-22 15:19:36 -0600, Nathan Bossart wrote: >> I think this is because the autoprewarm state was moved to a DSM segment, >> and DSM segments are detached before the on_shmem_exit callbacks are called >> during process exit. Moving apw_detach_shmem to the before_shmem_exit list >> seems to resolve the crashes. > > That seems plausible. Would still be nice to have at least this test ensure > that the shutdown code works. Perhaps just a check of the control file after > shutdown, ensuring that the state is "shutdowned" vs crashed? I'll give that a try. I'll also expand the comment above the before_shmem_exit() call. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Improving EXPLAIN's display of SubPlan nodes
I wrote:
> The main thing that's still missing compared to what is in the plan
> data structure is information about which Param is which. I think
> we have the subplan output Params relatively well covered through
> the expedient of listing them in the generated plan_name, but it's
> still not apparent to me how we could shoehorn subplan input
> Params into this (or whether it's worth doing).
Actually ... it looks like it probably isn't worth doing, because
it's already the case that we don't expose input Params as such.
EXPLAIN searches for the referent of an input Param and displays
that (cf find_param_referent()). Just for experimental purposes,
I wrote a follow-on patch to add printout of the parParam and args
list (attached, as .txt so the cfbot doesn't think it's a patch).
This produces results like
explain (verbose, costs off)
select array(select sum(x+y) s
from generate_series(1,3) y group by y order by s)
from generate_series(1,3) x;
QUERY PLAN
---
Function Scan on pg_catalog.generate_series x
Output: ARRAY(SubPlan 1 PASSING $0 := x.x)
^ added by delta patch
Function Call: generate_series(1, 3)
SubPlan 1
-> Sort
Output: (sum((x.x + y.y))), y.y
Sort Key: (sum((x.x + y.y)))
-> HashAggregate
Output: sum((x.x + y.y)), y.y
^^^ actual reference to $0
Group Key: y.y
-> Function Scan on pg_catalog.generate_series y
Output: y.y
Function Call: generate_series(1, 3)
(13 rows)
As you can see, it's not necessary to explain what $0 is because
that name isn't shown anywhere else --- the references to "x.x" in
the subplan are actually uses of $0.
So now I'm thinking that we do have enough detail in the present
proposal, and we just need to think about whether there's some
nicer way to present it than the particular spelling I used here.
One idea I considered briefly is to pull the same trick with
regards to output parameters --- that is, instead of adding all
the "returns $n" annotations to subplans, drill down and print
the subplan's relevant targetlist expression instead of "$n".
On balance I think that might be more confusing not less so,
though. SQL users are used to the idea that a sub-select can
"see" variables from the outer query, but not at all vice-versa.
I think it probably wouldn't be formally ambiguous, because
ruleutils already de-duplicates table aliases across the whole
tree, but it still seems likely to be confusing. Also, people
are already pretty used to seeing $n to represent the outputs
of InitPlans, and I've not heard many complaints suggesting
that we should change that.
regards, tom lane
diff --git a/src/backend/utils/adt/ruleutils.c
b/src/backend/utils/adt/ruleutils.c
index 9ee35640ba..b3da98a2f7 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -8911,9 +8911,30 @@ get_rule_expr(Node *node, deparse_context *context,
break;
}
if (subplan->useHashTable)
- appendStringInfo(buf, "HASHED %s)",
subplan->plan_name);
+ appendStringInfo(buf, "HASHED %s",
subplan->plan_name);
else
- appendStringInfo(buf, "%s)",
subplan->plan_name);
+ appendStringInfo(buf, "%s",
subplan->plan_name);
+ if (subplan->parParam)
+ {
+ ListCell *lc3;
+ ListCell *lc4;
+ bool first = true;
+
+ appendStringInfoString(buf, " PASSING
");
+ forboth(lc3, subplan->parParam, lc4,
subplan->args)
+ {
+ int paramid
= lfirst_int(lc3);
+ Node *arg = (Node *)
lfirst(lc4);
+
+ if (first)
+ first = false;
+ else
+
appendStringInfoString(buf, ", ");
+ appendStringInfo(buf, "$%d :=
", paramid);
+ get_rule_expr(arg, context,
showimplicit);
+ }
+ }
+
Re: Network failure may prevent promotion
Hi, On 2024-01-19 12:28:05 +0900, Michael Paquier wrote: > On Thu, Jan 18, 2024 at 03:42:28PM +0200, Heikki Linnakangas wrote: > > Given that commit 728f86fec6 that introduced this issue was not strictly > > required, perhaps we should just revert it for v16. > > Is there a point in keeping 728f86fec6 as well on HEAD? That does not > strike me as wise to keep that in the tree for now. If it needs to be > reworked, looking at this problem from scratch would be a safer > approach. IDK, I think we'll introduce this type of bug over and over if we don't fix it properly. Greetings, Andres Freund
Re: core dumps in auto_prewarm, tests succeed
Hi, On 2024-01-22 15:19:36 -0600, Nathan Bossart wrote: > On Mon, Jan 22, 2024 at 02:44:57PM -0600, Nathan Bossart wrote: > > On Mon, Jan 22, 2024 at 12:41:17PM -0800, Andres Freund wrote: > >> I noticed that I was getting core dumps while executing the tests, without > >> the > >> tests failing. Backtraces are vriations of this: > > > > Looking, thanks for the heads-up. > > I think this is because the autoprewarm state was moved to a DSM segment, > and DSM segments are detached before the on_shmem_exit callbacks are called > during process exit. Moving apw_detach_shmem to the before_shmem_exit list > seems to resolve the crashes. That seems plausible. Would still be nice to have at least this test ensure that the shutdown code works. Perhaps just a check of the control file after shutdown, ensuring that the state is "shutdowned" vs crashed? Greetings, Andres Freund
Re: core dumps in auto_prewarm, tests succeed
On Mon, Jan 22, 2024 at 02:44:57PM -0600, Nathan Bossart wrote: > On Mon, Jan 22, 2024 at 12:41:17PM -0800, Andres Freund wrote: >> I noticed that I was getting core dumps while executing the tests, without >> the >> tests failing. Backtraces are vriations of this: > > Looking, thanks for the heads-up. I think this is because the autoprewarm state was moved to a DSM segment, and DSM segments are detached before the on_shmem_exit callbacks are called during process exit. Moving apw_detach_shmem to the before_shmem_exit list seems to resolve the crashes. diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c index 9ea6c2252a..88c3a04109 100644 --- a/contrib/pg_prewarm/autoprewarm.c +++ b/contrib/pg_prewarm/autoprewarm.c @@ -165,7 +165,7 @@ autoprewarm_main(Datum main_arg) first_time = false; /* Set on-detach hook so that our PID will be cleared on exit. */ -on_shmem_exit(apw_detach_shmem, 0); +before_shmem_exit(apw_detach_shmem, 0); /* * Store our PID in the shared memory area --- unless there's already -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Optimizing nbtree ScalarArrayOp execution, allowing multi-column ordered scans, skip scan
On Fri, 19 Jan 2024 at 23:42, Peter Geoghegan wrote: > Thank you for your replies so far. > On Thu, Jan 18, 2024 at 11:39 AM Matthias van de Meent > wrote: > > I would agree with you if this was about new APIs and features, but > > here existing APIs are being repurposed without changing them. A > > maintainer of index AMs would not look at the commit title 'Enhance > > nbtree ScalarArrayOp execution' and think "oh, now I have to make sure > > my existing amsearcharray+amcanorder handling actually supports > > non-prefix arrays keys and return data in index order". > > This is getting ridiculous. > > It is quite likely that there are exactly zero affected out-of-core > index AMs. I don't count pgroonga as a counterexample (I don't think > that it actually fullfills the definition of a ). Basically, > "amcanorder" index AMs more or less promise to be compatible with > nbtree, down to having the same strategy numbers. So the idea that I'm > going to upset amsearcharray+amcanorder index AM authors is a > completely theoretical problem. The planner code evolved with nbtree, > hand-in-glove. And this is where I disagree with your (percieved) implicit argument that this should be and always stay this way. I don't mind changes in the planner for nbtree per se, but as I've mentioned before in other places, I really don't like how we handle amcanorder as if it were amisbtree. But it's not called that, so we shouldn't expect that to remain the case; and definitely not keep building on those expectations that it always is going to be the nbtree when amcanorder is set (or amsearcharray is set, or ..., or any combination of those that is currently used by btree). By keeping that expectation alive, this becomes a self-fulfilling prophecy, and I really don't like such restrictive self-fulfilling prophecies. It's nice that we have index AM feature flags, but if we only effectively allow one implementation for this set of flags by ignoring potential other users when changing behavior, then what is the API good for (apart from abstraction layering, which is nice)? /rant I'll see about the > I'm more than happy to add a reminder to the commit message about > adding a proforma listing to the compatibility section of the Postgres > 17 release notes. Can we actually agree on which theoretical index AM > types are affected first, though? Isn't it actually > amsearcharray+amcanorder+amcanmulticol external index AMs only? Do I > have that right? I think that may be right, but I could very well have built a btree-lite that doesn't have the historical baggage of having to deal with pages from before v12 (version 4) and some improvements that haven't made it to core yet. > BTW, how should we phrase this compatibility note, so that it can be > understood? It's rather awkward. Something like the following, maybe? """ Compatibility: The planner will now generate paths with array scan keys in any column for ordered indexes, rather than on only a prefix of the index columns. The planner still expects fully ordered data from those indexes. Historically, the btree AM couldn't output correctly ordered data for suffix array scan keys, which was "fixed" by prohibiting the planner from generating array scan keys without an equality prefix scan key up to that attribute. In this commit, the issue has been fixed, and the planner restriction has thus been removed as the only in-core IndexAM that reports amcanorder now supports array scan keys on any column regardless of what prefix scan keys it has. """ > > > As I said to Heikki, thinking about this some more is on my todo list. > > > I mean the way that this worked had substantial revisions on HEAD > > > right before I posted v9. v9 was only to fix the bit rot that that > > > caused. > > > > Then I'll be waiting for that TODO item to be resolved. > > My TODO item is to resolve the question of whether and to what extent > these two optimizations should be combined. It's possible that I'll > decide that it isn't worth it, for whatever reason. That's fine, this decision (and any work related to it) is exactly what I was referring to with that mention of the TODO. > > > Even single digit > > > thousand of array elements should be considered huge. Plus this code > > > path is only hit with poorly written queries. > > > > Would you accept suggestions? > > You mean that you want to have a go at it yourself? Sure, go ahead. > > I cannot promise that I'll accept your suggested revisions, but if > they aren't too invasive/complicated compared to what I have now, then > I will just accept them. I maintain that this isn't really necessary, > but it might be the path of least resistance at this point. Attached 2 patches: v11.patch-a and v11.patch-b. Both are incremental on top of your earlier set, and both don't allocate additional memory in the merge operation in non-assertion builds. patch-a is a trivial and clean implementation of mergesort, which tends to reduce the total number of
Re: Refactoring backend fork+exec code
Hi,
On 2024-01-10 14:35:52 +0200, Heikki Linnakangas wrote:
> Here's a patch that gets rid of AuxProcType. It's independent of the other
> patches in this thread; if this is committed, I'll rebase the rest of the
> patches over this and get rid of the new PMC_* enum.
>
> Three patches, actually. The first one fixes an existing comment that I
> noticed to be incorrect while working on this. I'll push that soon, barring
> objections. The second one gets rid of AuxProcType, and the third one
> replaces IsBackgroundWorker, IsAutoVacuumLauncherProcess() and
> IsAutoVacuumWorkerProcess() with checks on MyBackendType. So MyBackendType
> is now the primary way to check what kind of a process the current process
> is.
>
> 'am_walsender' would also be fairly straightforward to remove and replace
> with AmWalSenderProcess(). I didn't do that because we also have
> am_db_walsender and am_cascading_walsender which cannot be directly replaced
> with MyBackendType. Given that, it might be best to keep am_walsender for
> symmetry.
> @@ -561,13 +561,13 @@ static void ShmemBackendArrayAdd(Backend *bn);
> static void ShmemBackendArrayRemove(Backend *bn);
> #endif /* EXEC_BACKEND
> */
>
> -#define StartupDataBase()StartChildProcess(StartupProcess)
> -#define StartArchiver()
> StartChildProcess(ArchiverProcess)
> -#define StartBackgroundWriter() StartChildProcess(BgWriterProcess)
> -#define StartCheckpointer() StartChildProcess(CheckpointerProcess)
> -#define StartWalWriter() StartChildProcess(WalWriterProcess)
> -#define StartWalReceiver() StartChildProcess(WalReceiverProcess)
> -#define StartWalSummarizer() StartChildProcess(WalSummarizerProcess)
> +#define StartupDataBase()StartChildProcess(B_STARTUP)
> +#define StartArchiver() StartChildProcess(B_ARCHIVER)
> +#define StartBackgroundWriter() StartChildProcess(B_BG_WRITER)
> +#define StartCheckpointer() StartChildProcess(B_CHECKPOINTER)
> +#define StartWalWriter() StartChildProcess(B_WAL_WRITER)
> +#define StartWalReceiver() StartChildProcess(B_WAL_RECEIVER)
> +#define StartWalSummarizer() StartChildProcess(B_WAL_SUMMARIZER)
Not for this commit, but we ought to rip out these macros - all they do is to
make it harder to understand the code.
> @@ -5344,31 +5344,31 @@ StartChildProcess(AuxProcType type)
> errno = save_errno;
> switch (type)
> {
> - case StartupProcess:
> + case B_STARTUP:
> ereport(LOG,
> (errmsg("could not fork startup
> process: %m")));
> break;
> - case ArchiverProcess:
> + case B_ARCHIVER:
> ereport(LOG,
> (errmsg("could not fork
> archiver process: %m")));
> break;
> - case BgWriterProcess:
> + case B_BG_WRITER:
> ereport(LOG,
> (errmsg("could not fork
> background writer process: %m")));
> break;
> - case CheckpointerProcess:
> + case B_CHECKPOINTER:
> ereport(LOG,
> (errmsg("could not fork
> checkpointer process: %m")));
> break;
> - case WalWriterProcess:
> + case B_WAL_WRITER:
> ereport(LOG,
> (errmsg("could not fork WAL
> writer process: %m")));
> break;
> - case WalReceiverProcess:
> + case B_WAL_RECEIVER:
> ereport(LOG,
> (errmsg("could not fork WAL
> receiver process: %m")));
> break;
> - case WalSummarizerProcess:
> + case B_WAL_SUMMARIZER:
> ereport(LOG,
> (errmsg("could not fork WAL
> summarizer process: %m")));
> break;
Seems we should replace this with something slightly more generic one of these
days...
> diff --git a/src/backend/utils/activity/backend_status.c
> b/src/backend/utils/activity/backend_status.c
> index 1a1050c8da1..92f24db4e18 100644
> --- a/src/backend/utils/activity/backend_status.c
> +++ b/src/backend/utils/activity/backend_status.c
> @@ -257,17 +257,16 @@ pgstat_beinit(void)
> else
> {
> /* Must be an auxiliary process */
> -
Re: pg_stat_statements and "IN" conditions
> On Mon, Jan 22, 2024 at 06:07:27PM +0100, Dmitry Dolgov wrote: > > Please notice that at the moment, it's not being tested at all because > > of a patch-apply failure -- that's what the little triangular symbol > > means. The rest of the display concerns the test results from the > > last successfully-applied patch version. (Perhaps that isn't a > > particularly great UI design.) > > > > If you click on the triangle you find out > > > > == Applying patches on top of PostgreSQL commit ID > > b0f0a9432d0b6f53634a96715f2666f6d4ea25a1 === > > === applying patch > > ./v17-0001-Prevent-jumbling-of-every-element-in-ArrayExpr.patch > > patching file contrib/pg_stat_statements/Makefile > > Hunk #1 FAILED at 19. > > 1 out of 1 hunk FAILED -- saving rejects to file > > contrib/pg_stat_statements/Makefile.rej > > patching file contrib/pg_stat_statements/expected/merging.out > > patching file contrib/pg_stat_statements/meson.build > > Oh, I see, thanks. Give me a moment, will fix this. Here is it. >From ac8a7c93fbb72c469ca7128280f52024adb860ab Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Mon, 22 Jan 2024 21:11:15 +0100 Subject: [PATCH v18 1/4] Prevent jumbling of every element in ArrayExpr pg_stat_statements produces multiple entries for queries like SELECT something FROM table WHERE col IN (1, 2, 3, ...) depending on the number of parameters, because every element of ArrayExpr is jumbled. In certain situations it's undesirable, especially if the list becomes too large. Make an array of Const expressions contribute only the first/last elements to the jumble hash. Allow to enable this behavior via the new pg_stat_statements parameter query_id_const_merge with the default value off. Reviewed-by: Zhihong Yu, Sergey Dudoladov, Robert Haas, Tom Lane, Michael Paquier, Sergei Kornilov, Alvaro Herrera, David Geier Tested-by: Chengxi Sun --- contrib/pg_stat_statements/Makefile | 2 +- .../pg_stat_statements/expected/merging.out | 167 ++ contrib/pg_stat_statements/meson.build| 1 + .../pg_stat_statements/pg_stat_statements.c | 62 ++- contrib/pg_stat_statements/sql/merging.sql| 58 ++ doc/src/sgml/pgstatstatements.sgml| 57 +- src/backend/nodes/gen_node_support.pl | 21 ++- src/backend/nodes/queryjumblefuncs.c | 100 ++- src/backend/postmaster/postmaster.c | 3 + src/backend/utils/misc/postgresql.conf.sample | 1 - src/include/nodes/primnodes.h | 2 +- src/include/nodes/queryjumble.h | 9 +- 12 files changed, 458 insertions(+), 25 deletions(-) create mode 100644 contrib/pg_stat_statements/expected/merging.out create mode 100644 contrib/pg_stat_statements/sql/merging.sql diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile index 414a30856e4..03a62d685f3 100644 --- a/contrib/pg_stat_statements/Makefile +++ b/contrib/pg_stat_statements/Makefile @@ -19,7 +19,7 @@ LDFLAGS_SL += $(filter -lm, $(LIBS)) REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf REGRESS = select dml cursors utility level_tracking planning \ - user_activity wal entry_timestamp cleanup oldextversions + user_activity wal entry_timestamp cleanup oldextversions merging # Disabled because these tests require "shared_preload_libraries=pg_stat_statements", # which typical installcheck users do not have (e.g. buildfarm clients). NO_INSTALLCHECK = 1 diff --git a/contrib/pg_stat_statements/expected/merging.out b/contrib/pg_stat_statements/expected/merging.out new file mode 100644 index 000..1e58283afe8 --- /dev/null +++ b/contrib/pg_stat_statements/expected/merging.out @@ -0,0 +1,167 @@ +-- +-- Const merging functionality +-- +CREATE EXTENSION pg_stat_statements; +CREATE TABLE test_merge (id int, data int); +-- IN queries +-- No merging is performed, as a baseline result +SELECT pg_stat_statements_reset() IS NOT NULL AS t; + t +--- + t +(1 row) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11); + id | data ++-- +(0 rows) + +SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C"; +query| calls +-+--- + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) | 1 + SELECT pg_stat_statements_reset() IS NOT NULL AS t
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
Hi, On 2024-01-22 10:30:22 +1100, Peter Smith wrote: > 2024-01 Commitfest. > > Hi, This patch has a CF status of "Ready for Committer", but it is > currently failing some CFbot tests [1]. Please have a look and post an > updated version.. I think this failure is independent of this patch - by coincidence I just sent an email about the issue https://www.postgresql.org/message-id/20240122204117.swton324xcoodnyi%40awork3.anarazel.de a few minutes ago. Greetings, Andres Freund
Re: core dumps in auto_prewarm, tests succeed
On Mon, Jan 22, 2024 at 12:41:17PM -0800, Andres Freund wrote: > I noticed that I was getting core dumps while executing the tests, without the > tests failing. Backtraces are vriations of this: Looking, thanks for the heads-up. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
core dumps in auto_prewarm, tests succeed
Hi, I noticed that I was getting core dumps while executing the tests, without the tests failing. Backtraces are vriations of this: #0 0x00ca29cd in pg_atomic_read_u32_impl (ptr=0x7fe13497a004) at ../../../../../home/andres/src/postgresql/src/include/port/atomics/generic.h:48 #1 0x00ca2b08 in pg_atomic_read_u32 (ptr=0x7fe13497a004) at ../../../../../home/andres/src/postgresql/src/include/port/atomics.h:239 #2 0x00ca3c3d in LWLockAttemptLock (lock=0x7fe13497a000, mode=LW_EXCLUSIVE) at ../../../../../home/andres/src/postgresql/src/backend/storage/lmgr/lwlock.c:825 #3 0x00ca440c in LWLockAcquire (lock=0x7fe13497a000, mode=LW_EXCLUSIVE) at ../../../../../home/andres/src/postgresql/src/backend/storage/lmgr/lwlock.c:1264 #4 0x7fe130204ab4 in apw_detach_shmem (code=0, arg=0) at ../../../../../home/andres/src/postgresql/contrib/pg_prewarm/autoprewarm.c:788 #5 0x00c81c99 in shmem_exit (code=0) at ../../../../../home/andres/src/postgresql/src/backend/storage/ipc/ipc.c:276 #6 0x00c81a7c in proc_exit_prepare (code=0) at ../../../../../home/andres/src/postgresql/src/backend/storage/ipc/ipc.c:198 #7 0x00c819a8 in proc_exit (code=0) at ../../../../../home/andres/src/postgresql/src/backend/storage/ipc/ipc.c:111 #8 0x00bdd0b3 in BackgroundWorkerMain () at ../../../../../home/andres/src/postgresql/src/backend/postmaster/bgworker.c:841 #9 0x00be861d in do_start_bgworker (rw=0x341ff20) at ../../../../../home/andres/src/postgresql/src/backend/postmaster/postmaster.c:5756 #10 0x00be8a34 in maybe_start_bgworkers () at ../../../../../home/andres/src/postgresql/src/backend/postmaster/postmaster.c:5980 #11 0x00be4f9f in process_pm_child_exit () at ../../../../../home/andres/src/postgresql/src/backend/postmaster/postmaster.c:3039 #12 0x00be2de4 in ServerLoop () at ../../../../../home/andres/src/postgresql/src/backend/postmaster/postmaster.c:1765 #13 0x00be27bf in PostmasterMain (argc=4, argv=0x33dbba0) at ../../../../../home/andres/src/postgresql/src/backend/postmaster/postmaster.c:1475 #14 0x00aca326 in main (argc=4, argv=0x33dbba0) at ../../../../../home/andres/src/postgresql/src/backend/main/main.c:198 The most likely culprit seems to be: https://postgr.es/m/E1rQvjC-002Chd-Cd%40gemulon.postgresql.org The test encountering this is pg_prewarm/001_basic: (gdb) p DataDir $12 = 0x33ef8a0 "/srv/dev/build/postgres/m-dev-assert/testrun/pg_prewarm/001_basic/data/t_001_basic_main_data/pgdata" A secondary issue is that the tests suceed despite two segfaults. The problem here likely is that we don't have sufficient error handling during shutdowns: 2024-01-22 12:31:34.133 PST [3054823] LOG: background worker "logical replication launcher" (PID 3054836) exited with exit code 1 2024-01-22 12:31:34.443 PST [3054823] LOG: background worker "autoprewarm leader" (PID 3054835) was terminated by signal 11: Segmentation fault 2024-01-22 12:31:34.443 PST [3054823] LOG: terminating any other active server processes 2024-01-22 12:31:34.444 PST [3054823] LOG: abnormal database system shutdown 2024-01-22 12:31:34.469 PST [3054823] LOG: database system is shut down 2024-01-22 12:31:34.555 PST [3055133] LOG: starting PostgreSQL 17devel on x86_64-linux, compiled by gcc-14.0.0, 64-bit 2024-01-22 12:31:34.555 PST [3055133] LOG: listening on Unix socket "/tmp/p6XG0kQW9w/.s.PGSQL.60402" 2024-01-22 12:31:34.557 PST [3055148] LOG: database system was interrupted; last known up at 2024-01-22 12:31:33 PST 2024-01-22 12:31:34.557 PST [3055148] LOG: database system was not properly shut down; automatic recovery in progress 2024-01-22 12:31:34.558 PST [3055148] LOG: redo starts at 0/1531090 2024-01-22 12:31:34.559 PST [3055148] LOG: invalid record length at 0/1555F60: expected at least 24, got 0 2024-01-22 12:31:34.559 PST [3055148] LOG: redo done at 0/1555F38 system usage: CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s 2024-01-22 12:31:34.559 PST [3055146] LOG: checkpoint starting: end-of-recovery immediate wait 2024-01-22 12:31:34.570 PST [3055146] LOG: checkpoint complete: wrote 42 buffers (0.3%); 0 WAL file(s) added, 0 removed, 0 recycled; write=0.002 s, sync=0.001 s, total=0.011 s; sync files=0, longest=0.000 s, average=0.000 s; distance=147 kB, estimate=147 kB; lsn=0/1555F60, redo lsn=0/1555F60 2024-01-22 12:31:34.573 PST [3055133] LOG: database system is ready to accept connections ISTM that we shouldn't basically silently overlook shutdowns due to crashes in the tests. How to not do so is unfortunately not immediately obvious to me... Greetings, Andres Freund
Re: Improve WALRead() to suck data directly from WAL buffers when possible
Hi, On 2024-01-10 19:59:29 +0530, Bharath Rupireddy wrote: > + /* > + * Typically, we must not read a WAL buffer page that just got > + * initialized. Because we waited enough for the in-progress WAL > + * insertions to finish above. However, there can exist a slight > + * window after the above wait finishes in which the read > buffer page > + * can get replaced especially under high WAL generation rates. > After > + * all, we are reading from WAL buffers without any locks here. > So, > + * let's not count such a page in. > + */ > + phdr = (XLogPageHeader) page; > + if (!(phdr->xlp_magic == XLOG_PAGE_MAGIC && > + phdr->xlp_pageaddr == (ptr - (ptr % XLOG_BLCKSZ)) && > + phdr->xlp_tli == tli)) > + break; I still think that anything that requires such checks shouldn't be merged. It's completely bogus to check page contents for validity when we should have metadata telling us which range of the buffers is valid and which not. Greetings, Andres Freund
Re: Does redundant extension exist During faster COPY in PG16
Hi,
On 2024-01-22 19:54:00 +0800, 何柯文(渊云) wrote:
> I'm learning faster COPY of PG16. I have some questions about extension lock
> improvement.
> From ./src/backend/storage/buffer/bufmgr.c:1901 (ExtendBufferedRelShared)
> ```
> /*
> * Lock relation against concurrent extensions, unless requested not to.
> *
> * We use the same extension lock for all forks. That's unnecessarily
> * restrictive, but currently extensions for forks don't happen often
> * enough to make it worth locking more granularly.
> *
> * Note that another backend might have extended the relation by the time
> * we get the lock.
> */
> if (!(flags & EB_SKIP_EXTENSION_LOCK))
> {
> LockRelationForExtension(bmr.rel, ExclusiveLock);
> if (bmr.rel)
> bmr.smgr = RelationGetSmgr(bmr.rel);
> }
> ...
> smgrzeroextend(bmr.smgr, fork, first_block, extend_by, false);
> ```
> During concurrent extension, when we obtain the extension lock, we use
> smgrzeroextend() to extend relation files instead of searching fsm through
> GetPageWithFreeSpace(). Is this approach reasonable?
I think so, yes.
> During concurrent extensions, one backend bulk extend successfully, meaning
> that other backends waiting on extension lock have free pages to use. If
> all concurrent extend backends extend the relation file after getting the
> extension lock, the extension lock will be held (extention backends *
> smgrzeroextend() executing time).
If there's this much contention on the extension lock, there's no harm in
extending more - the space will be used soon. The alternatives would be a) to
search the FSM with the extension lock held, making contention worse, b) to
release the extension lock again if we couldn't immediately acquire it, search
the fsm, and retry if we couldn't find any free space, which would
substantially increase contention.
The FSM is the source of substantial contention, disabling it actually results
in substantial throughput increases. Vastly increasing the number of lookups
in the FSM would make that considerably worse, without a meaningful gain in
density.
Greetings,
Andres Freund
Re: make dist using git archive
On Mon Jan 22, 2024 at 1:31 AM CST, Peter Eisentraut wrote:
From 4b128faca90238d0a0bb6949a8050c2501d1bd67 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Sat, 20 Jan 2024 21:54:36 +0100
Subject: [PATCH v0] make dist uses git archive
---
GNUmakefile.in | 34 --
meson.build| 38 ++
2 files changed, 50 insertions(+), 22 deletions(-)
diff --git a/GNUmakefile.in b/GNUmakefile.in
index eba569e930e..3e04785ada2 100644
--- a/GNUmakefile.in
+++ b/GNUmakefile.in
@@ -87,29 +87,19 @@ update-unicode: | submake-generated-headers
submake-libpgport
distdir= postgresql-$(VERSION)
dummy = =install=
+GIT = git
+
dist: $(distdir).tar.gz $(distdir).tar.bz2
- rm -rf $(distdir)
-
-$(distdir).tar: distdir
- $(TAR) chf $@ $(distdir)
-
-.INTERMEDIATE: $(distdir).tar
-
-distdir-location:
- @echo $(distdir)
-
-distdir:
- rm -rf $(distdir)* $(dummy)
- for x in `cd $(top_srcdir) && find . \( -name CVS -prune \) -o \( -name
.git -prune \) -o -print`; do \
- file=`expr X$$x : 'X\./\(.*\)'`; \
- if test -d "$(top_srcdir)/$$file" ; then \
- mkdir "$(distdir)/$$file" && chmod 777 "$(distdir)/$$file"; \
- else \
- ln "$(top_srcdir)/$$file" "$(distdir)/$$file" >/dev/null 2>&1 \
- || cp "$(top_srcdir)/$$file" "$(distdir)/$$file"; \
- fi || exit; \
- done
- $(MAKE) -C $(distdir) distclean
+
+.PHONY: check-dirty-index
+check-dirty-index:
+ $(GIT) diff-index --quiet HEAD
+
+$(distdir).tar.gz: check-dirty-index
+ $(GIT) archive --format tar.gz --prefix $(distdir)/ HEAD -o $@
+
+$(distdir).tar.bz2: check-dirty-index
+ $(GIT) -c tar.tar.bz2.command='$(BZIP2) -c' archive --format tar.bz2
--prefix $(distdir)/ HEAD -o $@
distcheck: dist
rm -rf $(dummy)
diff --git a/meson.build b/meson.build
index c317144b6bc..f0d870c5192 100644
--- a/meson.build
+++ b/meson.build
@@ -3347,6 +3347,44 @@ run_target('help',
+###
+# Distribution archive
+###
+
+git = find_program('git', required: false, native: true, disabler: true)
+bzip2 = find_program('bzip2', required: false, native: true, disabler: true)
This doesn't need to be a disabler. git is fine as-is. See later
comment. Disablers only work like you are expecting when they are used
like how git is used. Once you call a method like .path(), all bets are
off.
+distdir = meson.project_name() + '-' + meson.project_version()
+
+check_dirty_index = run_target('check-dirty-index',
+ command: [git, 'diff-index', '--quiet', 'HEAD'])
Seems like you might want to add -C here too?
+
+tar_gz = custom_target('tar.gz',
+ build_always_stale: true,
+ command: [git, '-C', '@SOURCE_ROOT@', 'archive',
+'--format', 'tar.gz',
+'--prefix', distdir + '/',
+'-o', '@BUILD_ROOT@/@OUTPUT@',
+'HEAD', '.'],
+ install: false,
+ output: distdir + '.tar.gz',
+)
+
+tar_bz2 = custom_target('tar.bz2',
+ build_always_stale: true,
+ command: [git, '-C', '@SOURCE_ROOT@', '-c', 'tar.tar.bz2.command=' +
bzip2.path() + ' -c', 'archive',
+'--format', 'tar.bz2',
+'--prefix', distdir + '/',
-'-o', '@BUILD_ROOT@/@OUTPUT@',
+'-o', join_paths(meson.build_root(), '@OUTPUT@'),
This will generate the tarballs in the build directory. Do the same for
the previous target. Tested locally.
+'HEAD', '.'],
+ install: false,
+ output: distdir + '.tar.bz2',
+)
The bz2 target should be wrapped in an `if bzip2.found()`. It is
possible for git to be found, but not bzip2. I might also define the bz2
command out of line. Also, you may want to add
these programs to meson_options.txt for overriding, even though the
"meson-ic" way is to use a machine file.
+
+alias_target('pgdist', [check_dirty_index, tar_gz, tar_bz2])
Are you intending for the check_dirty_index target to prohibit the other
two targets from running? Currently that is not the case. If it is what
you intend, use a stamp file or something to indicate a relationship.
Alternatively, inline the git diff-index into the other commands. These
might also do better as external scripts. It would reduce duplication
between the autotools and Meson builds.
+
+
+
###
# The End, The End, My Friend
###
I am not really following why we can't use the builtin Meson dist
command. The only difference from my testing is it doesn't use
a --prefix argument.
--
Tristan Partin
Neon (https://neon.tech)
Multiple startup messages over the same connection
Hello, A question about protocol design - would it be possible to extend the protocol, so it can handle multiple startup / authentication messages over a single connection? Are there any serious obstacles? (possible issues with re-initialization of backends, I guess?) If that is possible, it could improve one important edge case - where you have to talk to multiple databases on a single host currently, you need to open a separate connection to each of them. In some cases (multitenancy for example), you may have thousands of databases on a host, which leads to inefficient connection utilization on clients (on the db side too). A lot of other RDBMSes don't have this limitation. thank you, -Vladimir Churyukin
Re: psql: Allow editing query results with \gedit
Re: Pavel Stehule > Introduction of \gedit is interesting idea, but in this form it looks too > magic > > a) why the data are in JSON format, that is not native for psql (minimally > now) Because we need something machine-readable. CSV would be an alternative, but that is hardly human-readable. > b) the implicit transformation to UPDATEs and the next evaluation can be > pretty dangerous. You can always run it in a transaction: begin; select * from tbl \gedit commit; Alternatively, we could try to make it not send the commands right away - either by putting them into the query buffer (that currently work only for single commands without any ';') or by opening another editor. Doable, but perhaps the transaction Just Works? > The concept of proposed feature is interesting, but the name \gedit is too > generic, maybe too less descriptive for this purpose > > Maybe \geditupdates can be better - but still it can be dangerous and slow > (without limits) An alternative I was considering was \edittable, but that would put more emphasis on "table" when it's actually more powerful than that, it works on a query result. (Similar in scope to updatable views.) > In the end I am not sure if I like it or dislike it. Looks dangerous. I can > imagine possible damage when some people will see vi first time and will > try to finish vi, but in this command, it will be transformed to executed > UPDATEs. If you close the editor with touching the file, nothing will be sent. And if you mess up the file, it will complain. I think it's unlikely that people who end up in an editor they can't operate would be able to modify JSON in a way that is still valid. Also, \e has the same problem. Please don't let "there are users who don't know what they are doing" spoil the idea. > More generating UPDATEs without knowledge of table structure > (knowledge of PK) can be issue (and possibly dangerous too), and you cannot > to recognize PK from result of SELECT (Everywhere PK is not "id" and it is > not one column). It *does* retrieve the proper PK from the table. All updates are based on the PK. Re: Tom Lane > > Introduction of \gedit is interesting idea, but in this form it looks too > > magic > > Yeah, I don't like it either --- it feels like something that belongs > in an ETL tool not psql. I tried to put it elsewhere first, starting with pspg: https://github.com/okbob/pspg/issues/200 The problem is that external programs like the pager neither have access to the query string/table name, nor the database connection. ETL would also not quite fit, this is meant for interactive use. > The sheer size of the patch shows how far > afield it is from anything that psql already does, necessitating > writing tons of stuff that was not there before. I've been working on this for several months, so it's already larger than the MVP would be. It does have features like (key=col1,col2) that could be omitted. > The bits that try > to parse the query to get necessary information seem particularly > half-baked. Yes, that's not pretty, and I'd be open for suggestions on how to improve that. I was considering: 1) this (dumb query parsing) 2) EXPLAIN the query to get the table 3) use libpq's PQftable The problem with 2+3 is that on views and partitioned tables, this would yield the base table name, not the table name used in the query. 1 turned out to be the most practical, and worked for me so far. If the parse tree would be available, using that would be much better. Should we perhaps add something like "explain (parse) select...", or add pg_parsetree(query) function? > Yup -- you'd certainly want some way of confirming that you actually > want the changes applied. Our existing edit commands load the edited > string back into the query buffer, where you can \r it if you don't > want to run it. But I fear that the results of this operation would > be too long for that to be workable. The results are as long as you like. The intended use case would be to change just a few rows. As said above, I was already thinking of making it user-confirmable, just the current version doesn't have it. > It does look like it's trying to find out the pkey from the system > catalogs ... however, it's also accepting unique constraints without > a lot of thought about the implications of NULLs. Right, the UNIQUE part doesn't take care of NULLs yet. Will fix that. (Probably by erroring out if any key column is NULL.) > Even if you have > a pkey, it's not exactly clear to me what should happen if the user > changes the contents of a pkey field. That could be interpreted as > either an INSERT or UPDATE, I think. A changed PK will be interpreted as DELETE + INSERT. (I shall make that more clear in the documentation.) > Also, while I didn't read too closely, it's not clear to me how the > code could reliably distinguish INSERT vs UPDATE vs DELETE cases --- > surely we're not going to try to put a "diff" engine into this, and > even if we did, diff
Re: [17] CREATE SUBSCRIPTION ... SERVER
On Mon, 2024-01-22 at 18:41 +0530, Ashutosh Bapat wrote: > 0002 adds a prefix "regress_" to almost every object that is created > in foreign_data.sql. psql \dew outputs the owner, which in the case of a built-in FDW is the bootstrap superuser, which is not a stable name. I used the prefix to exclude the built-in FDW -- if you have a better suggestion, please let me know. (Though reading below, we might not even want a built-in FDW.) > Dummy FDW makes me nervous. The way it's written, it may grow into a > full-fledged postgres_fdw and in the process might acquire the same > concerns that postgres_fdw has today. But I will study the patches > and > discussion around it more carefully. I introduced that based on this comment[1]. I also thought it fit with your previous suggestion to make it work with postgres_fdw, but I suppose it's not required. We could just not offer the built-in FDW, and expect users to either use postgres_fdw or create their own dummy FDW. > I enhanced the postgres_fdw TAP test to use foreign table. Please see > the attached patch. It works as expected. Of course a follow-on work > will require linking the local table and its replica on the publisher > table so that push down will work on replicated tables. But the > concept at least works with your changes. Thanks for that. Thank you, I'll include it in the next patch set. > I am not sure we need a full-fledged TAP test for testing > subscription. I wouldn't object to it, but TAP tests are heavy. It > should be possible to write the same test as a SQL test by creating > two databases and switching between them. Do you think it's worth > trying that way? I'm not entirely sure what you mean here, but I am open to test simplifications if you see an opportunity. Regards, Jeff Davis > [1] https://www.postgresql.org/message-id/172273.1693403385%40sss.pgh.pa.us
Re: Adding facility for injection points (or probe points?) for more advanced tests
On 22/01/2024 18:08, Heikki Linnakangas wrote: I wrote the attached patch to enable injection points in the Cirrus CI config, to run the injection tests I wrote for a GIN bug today [1]. But that led to a crash in the asan-enabled build [2]. I didn't investigate it yet. Pushed a fix for the crash. What do you think of enabling this in the Cirrus CI config? -- Heikki Linnakangas Neon (https://neon.tech)
Re: partitioning and identity column
On 22.01.24 13:23, Ashutosh Bapat wrote:
if (newdef->identity)
{
Assert(!is_partioning);
/*
* Identity is never inherited. The new column can have an
* identity definition, so we always just take that one.
*/
def->identity = newdef->identity;
}
Thoughts?
That code block already has Assert(!is_partition) at line 3085. I
thought that Assert is enough.
Ok. Maybe just rephrase that comment somehow then?
There's another thing I found. The file isn't using
check_stack_depth() in the function which traverse inheritance
hierarchies. This isn't just a problem of the identity related
function but most of the functions in that file. Do you think it's
worth fixing it?
I suppose the number of inheritance levels is usually not a problem for
stack depth?
Re: Built-in CTYPE provider
On 18.01.24 23:03, Jeff Davis wrote: On Thu, 2024-01-18 at 13:53 +0100, Peter Eisentraut wrote: I think that would be a terrible direction to take, because it would regress the default sort order from "correct" to "useless". I don't agree that the current default is "correct". There are a lot of ways it can be wrong: * the environment variables at initdb time don't reflect what the users of the database actually want * there are so many different users using so many different applications connected to the database that no one "correct" sort order exists * libc has some implementation quirks * the version of Unicode that libc is based on is not what you expect * the version of libc is not what you expect These are arguments why the current defaults are not universally perfect, but I'd argue that they are still most often the right thing as the default. Aside from the overall message this sends about how PostgreSQL cares about locales and Unicode and such. Unicode is primarily about the semantics of characters and their relationships. The patches I propose here do a great job of that. Collation (relationships between *strings*) is a part of Unicode, but not the whole thing or even the main thing. I don't get this argument. Of course, people care about sorting and sort order. Whether you consider this part of Unicode or adjacent to it, people still want it. Maybe you don't intend for this to be the default provider? I am not proposing that this provider be the initdb-time default. ok But then who would really use it? I mean, sure, some people would, but how would you even explain, in practice, the particular niche of users or use cases? It's for users who want to respect Unicode support text from international sources in their database; but are not experts on the subject and don't know precisely what they want or understand the consequences. If and when such users do notice a problem with the sort order, they'd handle it at that time (perhaps with a COLLATE clause, or sorting in the application). Vision: * ICU offers COLLATE UNICODE, locale tailoring, case-insensitive matching, and customization with rules. It's the solution for everything from "slightly more advanced" to "very advanced". I am astonished by this. In your world, do users not want their text data sorted? Do they not care what the sort order is? You consider UCA sort order an "advanced" feature?
Re: make dist using git archive
On 22.01.24 13:10, Junwang Zhao wrote: I played this with meson build on macOS, the packages are generated in source root but not build root, I'm sure if this is by design but I think polluting *working directory* is not good. Yes, it's not good, but I couldn't find a way to make it work. This is part of the complications with meson I referred to. The @BUILD_ROOT@ placeholder in custom_target() is apparently always a relative path, but it doesn't know that git -C changes the current directory. Another thing I'd like to point out is, should we also introduce *git commit* or maybe *git tag* to package name, something like: git archive --format tar.gz --prefix postgresql-17devel/ HEAD -o postgresql-17devel-`git rev-parse --short HEAD`.tar.gz git archive --format tar.gz --prefix postgresql-17devel/ HEAD -o postgresql-`git describe --tags`.tar.gz I'm not sure why we would need it built-in. It can be done by hand, of course.
Re: Permute underscore separated components of columns before fuzzy matching
Arne Roland writes: > Thank you for bringing that to my attention. Is there a way to subscribe > to cf-bot failures? I don't know of any push notification support in cfbot, but you can bookmark the page with your own active patches, and check it periodically: http://commitfest.cputube.org/arne-roland.html (For others, click on your own name in the main cfbot page's entry for one of your patches to find out how it spelled your name for this purpose.) regards, tom lane
Re: psql: Allow editing query results with \gedit
Pavel Stehule writes: > I would have forms like FoxPro, I would have a grid like FoxPro, but not in > psql, and I would not develop it :-) Yeah, that's something that was also bothering me, but I failed to put my finger on it. "Here's some JSON, edit it, and don't forget to keep the quoting correct" does not strike me as a user-friendly way to adjust data content. A spreadsheet-like display where you can change data within cells seems like a far better API, although I don't want to write that either. This kind of API would not readily support INSERT or DELETE cases, but TBH I think that's better anyway --- you're adding too much ambiguity in pursuit of a very secondary use-case. The stated complaint was "it's too hard to build UPDATE commands", which I can sympathize with. (BTW, I wonder how much of this already exists in pgAdmin.) regards, tom lane
Re: Teach predtest about IS [NOT] proofs
James Coleman writes:
> 0001 does the initial pure refactor. 0003 makes a lot of modifications
> to what we can prove about implication and refutation. Finally, 0003
> isn't intended to be committed, but attempts to validate more
> holistically that none of the changes creates any invalid proofs
> beyond the mostly happy-path tests added in 0004.
> I ended up not tackling changing how test_predtest tests run for now.
> That's plausibly still useful, and I'd be happy to add that if you
> generally agree with the direction of the patch and with that
> abstraction being useful.
> I added some additional verifications to the test_predtest module to
> prevent additional obvious flaws.
I looked through 0001 and made some additional cosmetic changes,
mostly to get comments closer to the associated code; I also
ran pgindent on it (see v5-0001 attached). That seems pretty
committable to me at this point. I also like your 0002 additions to
test_predtest.c (although why the mixture of ERROR and WARNING?
ISTM they should all be WARNING, so we can press on with the test).
One other thought is that maybe separating out
predicate_implied_not_null_by_clause should be part of 0001?
I'm less excited about the rest of v4-0002.
@@ -740,6 +747,16 @@ predicate_refuted_by_recurse(Node *clause, Node *predicate,
!weak))
return true;
+/*
+ * Because weak refutation expands the allowed outcomes for B
+ * from "false" to "false or null", we can additionally prove
+ * weak refutation in the case that strong refutation is proven.
+ */
+if (weak && not_arg &&
+predicate_implied_by_recurse(predicate, not_arg,
+ true))
+return true;
+
switch (pclass)
{
case CLASS_AND:
I don't buy this bit at all. If the prior recursive call fails to
prove weak refutation in a case where strong refutation holds, how is
that not a bug lower down? Moreover, in order to mask such a bug,
you're doubling the time taken by failed proofs, which is an
unfortunate thing --- we don't like spending a lot of time on
something that fails to improve the plan.
@@ -1138,32 +1155,114 @@ predicate_implied_by_simple_clause(Expr *predicate,
Node *clause,
Assert(list_length(op->args) == 2);
rightop = lsecond(op->args);
-/*
- * We might never see a null Const here, but better check
- * anyway
- */
-if (rightop && IsA(rightop, Const) &&
-!((Const *) rightop)->constisnull)
+if (rightop && IsA(rightop, Const))
{
+Const*constexpr = (Const *) rightop;
Node *leftop = linitial(op->args);
-if (DatumGetBool(((Const *) rightop)->constvalue))
-{
-/* X = true implies X */
-if (equal(predicate, leftop))
-return true;
-}
+if (constexpr->constisnull)
+return false;
+
+if (DatumGetBool(constexpr->constvalue))
+return equal(predicate, leftop);
else
-{
-/* X = false implies NOT X */
-if (is_notclause(predicate) &&
-equal(get_notclausearg(predicate), leftop))
-return true;
-}
+return is_notclause(predicate) &&
+equal(get_notclausearg(predicate), leftop);
}
}
}
break;
I don't understand what this bit is doing ... and the fact that
the patch removes all the existing comments and adds none isn't
helping that. What it seems to mostly be doing is adding early
"return false"s, which I'm not sure is a good thing, because
it seems possible that operator_predicate_proof could apply here.
+case IS_UNKNOWN:
+/*
+ * When the clause is in the form "foo IS UNKNOWN" then
+ * we can prove weak implication of a predicate that
+ * is strict for "foo" and negated. This doesn't work
+ * for strong implication since if "foo" is "null" then
+ * the predicate will evaluate to "null" rather than
+ * "true".
+ */
The phrasing of this comment seems randomly inconsistent with others
making similar arguments.
+
Re: Network failure may prevent promotion
On Thu, Jan 18, 2024 at 10:42 PM Heikki Linnakangas wrote: > Given that commit 728f86fec6 that introduced this issue was not strictly > required, perhaps we should just revert it for v16. +1 for the revert. This issue should be fixed in the upcoming minor release since it might cause unexpected delays in failover times. Regards, -- Fujii Masao
Re: the s_lock_stuck on perform_spin_delay
Robert Haas writes: > On Mon, Jan 22, 2024 at 12:13 PM Andy Fan wrote: >> > On Mon, Jan 22, 2024 at 11:58 AM Andy Fan wrote: >> >> I get your point! Acquiring an already held spinlock in quickdie is >> >> unlikely to happen, but since our existing infrastructure can handle it, >> >> then there is no reason to bypass it. >> > >> > No, the existing infrastructure cannot handle that at all. >> >> Actually I mean we can handle it without 0003. am I still wrong? >> Without the 0003, if we acquiring the spin lock which is held by >> ourself already. VerifyNoSpinLocksHeld in SpinLockAcquire should catch >> it. > > But that's only going to run in assert-only builds. The whole point of > the patch set is to tell developers that there are bugs in the code > that need fixing, not to catch problems that actually occur in > production. I see. As to this aspect, then yes. -- Best Regards Andy Fan
Re: the s_lock_stuck on perform_spin_delay
On Mon, Jan 22, 2024 at 12:13 PM Andy Fan wrote: > > On Mon, Jan 22, 2024 at 11:58 AM Andy Fan wrote: > >> I get your point! Acquiring an already held spinlock in quickdie is > >> unlikely to happen, but since our existing infrastructure can handle it, > >> then there is no reason to bypass it. > > > > No, the existing infrastructure cannot handle that at all. > > Actually I mean we can handle it without 0003. am I still wrong? > Without the 0003, if we acquiring the spin lock which is held by > ourself already. VerifyNoSpinLocksHeld in SpinLockAcquire should catch > it. But that's only going to run in assert-only builds. The whole point of the patch set is to tell developers that there are bugs in the code that need fixing, not to catch problems that actually occur in production. -- Robert Haas EDB: http://www.enterprisedb.com
Re: XLog size reductions: smaller XLRec block header for PG17
On Mon, Jan 22, 2024 at 5:38 AM Aleksander Alekseev wrote: > I don't think that closing CF entries purely due to inactivity is a > good practice (neither something we did before) as long as there is > code, it applies, etc. There are a lot of patches and few people > working on them. Inactivity in a given thread doesn't necessarily > indicate lack of interest, more likely lack of resources. If the CF entry doesn't serve the purpose of allowing someone to find patches in need of review, it's worse than useless. Patches that aren't getting reviewed should stay in the CF until they do, or until we make a collective decision that we don't care about or want that particular patch enough to bother. But patches that are not ready for review need to get out until such time as they are. Otherwise they're just non-actionable clutter. And unfortunately we have so much of that in the CF app now that finding things that really need review is time consuming and difficult. That REALLY needs to be cleaned up. In the case of this particular patch, I think the problem is that there's no consensus on the design. There's not a ton of debate on this thread, but thread [1] linked in the original post contains a lot of vigorous debate about what the right thing to do is here and I don't believe we reached any meeting of the minds. In light of that lack of agreement, I'm honestly a bit confused why Matthias even found it worthwhile to submit this on a new thread. I think we all agree with him that there's room for improvement here, but we don't agree on what particular form that improvement should take, and as long as that agreement is lacking, I find it hard to imagine anything getting committed. The task right now is to get agreement on something, and leaving the CF entry open longer isn't going make the people who have already expressed opinions start agreeing with each other more than they do already. It looks like I never replied to https://www.postgresql.org/message-id/20221019192130.ebjbycpw6bzjry4v%40awork3.anarazel.de but, FWIW, I agree with Andres that applying the same technique to multiple fields that are stored together (DB OID, TS OID, rel #, block #) is unlikely in practice to produce many cases that regress. But the question for this thread is really more about whether we're OK with using ad-hoc bit swizzling to reduce the size of xlog records or whether we want to insist on the use of a uniform varint encoding. Heikki and Andres both seem to favor the latter. IIRC, I was initially more optimistic about ad-hoc bit swizzling being a potentially acceptable technique, but I'm not convinced enough about it to argue against two very smart committers both of whom know more about micro-optimizing performance than I do, and nobody else seems to making this argument on this thread either, so I just don't really see how this patch is ever going to go anywhere in its current form. -- Robert Haas EDB: http://www.enterprisedb.com
Commitfest 2024-01 third week update
Hi, Here's a quick status report after the third week: Status summary: status| w1 | w2 | w3 ---+---++-- Needs review: |238 | 213| 181 Waiting on Author: | 44 | 46 | 52 Ready for Committer:| 27 | 27 | 26 Committed:| 36 | 46 | 57 Moved to next CF | 1 | 3 | 4 Withdrawn:| 2 | 4 | 12 Returned with Feedback: | 3 | 12 | 18 Rejected: | 1 | 1 | 2 Total: | 352| 352 | 352 If you have submitted a patch and it's in "Waiting for author" state, please aim to get it to "Needs review" state soon if you can, as that's where people are most likely to be looking for things to review. I have pinged most threads that are in "Needs review" state and don't apply, compile warning-free, or pass check-world. I'll do some more of that sort of thing. I have sent a private mail through commitfest to patch owners who have submitted one or more patches but have not picked any of the patches for review. I have sent out mails for which there has been no activity for a long time, please respond to the mails if you are planning to continue to work on it or if you are planning to work on it in the next commitfest, please move it to the next commitfest. If there is no response I'm planning to return these patches and it can be added again when it will be worked upon actively. Regards, Vignesh
Re: psql: Allow editing query results with \gedit
po 22. 1. 2024 v 17:34 odesílatel David G. Johnston < david.g.johns...@gmail.com> napsal: > On Mon, Jan 22, 2024 at 8:06 AM Christoph Berg wrote: > >> Assuming a SELECT statement reading from a single table, it is quite an >> effort to transform that statement to an UPDATE statement on that table, >> perhaps to fix a typo that the user has spotted in the query result. >> >> > Building off the other comments, I'd suggest trying to get rid of the > intermediate JSOn format and also just focus on a single row at any given > time. > > For an update the first argument to the metacommand could be the unique > key value present in the previous result. The resultant UPDATE would just > put that into the where clause and every other column in the result would > be a SET clause column with the thing being set the current value, ready to > be edited. > > DELETE would be similar but without the need for a SET clause. > > INSERT can produce a template INSERT (cols) VALUES ... command with some > smarts regarding auto incrementing keys and placeholder values. > > David J. > Can you imagine using it? I like psql, I like term applications, my first database was FoxPro, but for dataeditation I am almost sure so I don't want to use psql. I can imagine enhancing the current \gexec command because it is executed directly without possibility to edit. I see valuable some special clause "edit" like \gexec_edit or \gexec(edit) or \gexec edit This is like current gexec but with possibility to edit the result in editor and with possibility to finish without saving. Then we can introduce SQL functions UpdateTemplate(cols text[], rowvalue), InsertTemplate, ... and then you can write SELECT UpdateTemplace(ARRAY['a','b','c'], foo) FROM foo WHERE id IN (1,2) \gexec_with_edit But still looks strange to me - like we try reintroduce of necessity sed or awk to SQL and psql I would have forms like FoxPro, I would have a grid like FoxPro, but not in psql, and I would not develop it :-)
Re: Permute underscore separated components of columns before fuzzy matching
Thank you for bringing that to my attention. Is there a way to subscribe
to cf-bot failures?
Apparently I confused myself with my naming. I attached a patch that
fixes the bug (at least at my cassert test-world run).
Regards
Arne
On 2024-01-22 06:38, Peter Smith wrote:
2024-01 Commitfest.
Hi, This patch has a CF status of "Needs Review" [1], but it seems
like there were CFbot test failures last time it was run [2]. Please
have a look and post an updated version if necessary.
==
[1] https://commitfest.postgresql.org/46/4282/
[2] https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/4282
Kind Regards,
Peter Smith.diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 34a0ec5901..c416be2ea0 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -579,32 +579,13 @@ GetCTEForRTE(ParseState *pstate, RangeTblEntry *rte, int rtelevelsup)
return NULL;/* keep compiler quiet */
}
-/*
- * updateFuzzyAttrMatchState
- * Using Levenshtein distance, consider if column is best fuzzy match.
- */
static void
-updateFuzzyAttrMatchState(int fuzzy_rte_penalty,
- FuzzyAttrMatchState *fuzzystate, RangeTblEntry *rte,
- const char *actual, const char *match, int attnum)
+updateFuzzyAttrMatchStateSingleString(int fuzzy_rte_penalty,
+ FuzzyAttrMatchState *fuzzystate, RangeTblEntry *rte,
+ const char *actual, const char *match, int attnum, int matchlen)
{
- int columndistance;
- int matchlen;
-
- /* Bail before computing the Levenshtein distance if there's no hope. */
- if (fuzzy_rte_penalty > fuzzystate->distance)
- return;
-
- /*
- * Outright reject dropped columns, which can appear here with apparent
- * empty actual names, per remarks within scanRTEForColumn().
- */
- if (actual[0] == '\0')
- return;
-
/* Use Levenshtein to compute match distance. */
- matchlen = strlen(match);
- columndistance =
+ int columndistance =
varstr_levenshtein_less_equal(actual, strlen(actual), match, matchlen,
1, 1, 1,
fuzzystate->distance + 1
@@ -667,6 +648,142 @@ updateFuzzyAttrMatchState(int fuzzy_rte_penalty,
}
}
+static void putUnderscores(char* string, int start, int underscore_amount, int len) {
+ for (int i = 0; start + i < len && i < underscore_amount; i++)
+ string[start + i] = '_';
+}
+
+/*
+ * updateFuzzyAttrMatchState
+ * Using Levenshtein distance, consider if column is best fuzzy match.
+ */
+static void
+updateFuzzyAttrMatchState(int fuzzy_rte_penalty,
+ FuzzyAttrMatchState *fuzzystate, RangeTblEntry *rte,
+ const char *actual, const char *match, int attnum)
+{
+ /* Memory segment to store the current permutation of the match string. */
+ char* tmp_match;
+ int matchlen = strlen(match);
+ /* We keep track how many permutations we have already processed, to avoid long runtimes. */
+ int underscore_permutations_count = 0;
+ /* The location the underscore we currently process within the match string. */
+ int underscore_current = 1;
+ /* Variables to track the amount of underscores delimiting sections */
+ int underscore_amount = 1;
+ int underscore_second_amount = 1;
+
+ /* Bail before computing the Levenshtein distance if there's no hope. */
+ if (fuzzy_rte_penalty > fuzzystate->distance)
+ return;
+
+ /*
+ * Outright reject dropped columns, which can appear here with apparent
+ * empty actual names, per remarks within scanRTEForColumn().
+ */
+ if (actual[0] == '\0')
+ return;
+
+ updateFuzzyAttrMatchStateSingleString(fuzzy_rte_penalty, fuzzystate, rte, actual, match, attnum, matchlen);
+ /* We don't want to permute zero length strings, so check whether the string starts with an underscore. */
+ if (match[0] == '_') {
+ while (underscore_current < matchlen - 1 && match[underscore_current] == '_') {
+ underscore_current++;
+ }
+ }
+ /* Advance to the next underscore. We do this once here to avoid pallocing, if the string does't contain an underscore at all. */
+ while (underscore_current < matchlen - 1 && match[underscore_current] != '_') {
+ underscore_current++;
+ }
+ /*
+ * Check for permuting up to three sections separated by underscores.
+ *
+ * We count the number of underscores here, because we want to know whether we should consider
+ * permuting underscore separated sections.
+ */
+ if (underscore_current < matchlen - 1) {
+ tmp_match = palloc(matchlen + 1);
+ tmp_match[matchlen] = '\0';
+ while (underscore_permutations_count < 300 && underscore_current < matchlen - 1) {
+ /*
+ * If sections contain more than one underscore, we want to swap the sections separated by more than one instead.
+ * There would be no point in swapping zero length strings around.
+ * So we check how many consecutive underscores we have here.
+ */
+ underscore_amount = 1;
+ while (underscore_current + underscore_amount < matchlen && match[underscore_current + underscore_amount] == '_') {
+underscore_amount++;
+
Re: the s_lock_stuck on perform_spin_delay
Robert Haas writes: > On Mon, Jan 22, 2024 at 11:58 AM Andy Fan wrote: >> I get your point! Acquiring an already held spinlock in quickdie is >> unlikely to happen, but since our existing infrastructure can handle it, >> then there is no reason to bypass it. > > No, the existing infrastructure cannot handle that at all. Actually I mean we can handle it without 0003. am I still wrong? Without the 0003, if we acquiring the spin lock which is held by ourself already. VerifyNoSpinLocksHeld in SpinLockAcquire should catch it. -- Best Regards Andy Fan
Re: Improving EXPLAIN's display of SubPlan nodes
Hi Aleksander and Tom I do confirm that I requested to get this information, in order to recover the formula to filter on. Thanks to both of you Chantal Le 22/01/2024 à 18:07, Tom Lane a écrit : Aleksander Alekseev writes: Although something like: ``` + Filter: (ANY (base_tbl.a = $1) FROM SubPlan 1 (returns $1)) + SubPlan 1 (returns $1) ``` ... arguably doesn't give much more information to the user comparing to what we have now: ``` - Filter: (SubPlan 1) - SubPlan 1 ``` Yeah, I would probably not have thought to do this on my own; but we had an actual user request for it. I think arguably the main benefit is to confirm "yes, this is the sub-select you think it is". The main thing that's still missing compared to what is in the plan data structure is information about which Param is which. I think we have the subplan output Params relatively well covered through the expedient of listing them in the generated plan_name, but it's still not apparent to me how we could shoehorn subplan input Params into this (or whether it's worth doing). regards, tom lane
Re: pg_stat_statements and "IN" conditions
> On Mon, Jan 22, 2024 at 11:35:22AM -0500, Tom Lane wrote: > Dmitry Dolgov <9erthali...@gmail.com> writes: > >> On Mon, Jan 22, 2024 at 05:33:26PM +1100, Peter Smith wrote: > >> Hi, This patch has a CF status of "Needs Review" [1], but it seems > >> there was a CFbot test failure last time it was run [2]. Please have a > >> look and post an updated version if necessary. > >> > >> == > >> [1] https://commitfest.postgresql.org/46/2837/ > >> [2] https://cirrus-ci.com/task/6688578378399744 > > > It's the same failing pipeline Vignesh C was talking above. I've fixed > > the issue in the latest patch version, but looks like it wasn't picked > > up yet (from what I understand, the latest build for this CF is 8 weeks > > old). > > Please notice that at the moment, it's not being tested at all because > of a patch-apply failure -- that's what the little triangular symbol > means. The rest of the display concerns the test results from the > last successfully-applied patch version. (Perhaps that isn't a > particularly great UI design.) > > If you click on the triangle you find out > > == Applying patches on top of PostgreSQL commit ID > b0f0a9432d0b6f53634a96715f2666f6d4ea25a1 === > === applying patch > ./v17-0001-Prevent-jumbling-of-every-element-in-ArrayExpr.patch > patching file contrib/pg_stat_statements/Makefile > Hunk #1 FAILED at 19. > 1 out of 1 hunk FAILED -- saving rejects to file > contrib/pg_stat_statements/Makefile.rej > patching file contrib/pg_stat_statements/expected/merging.out > patching file contrib/pg_stat_statements/meson.build Oh, I see, thanks. Give me a moment, will fix this.
Re: Improving EXPLAIN's display of SubPlan nodes
Aleksander Alekseev writes: > Although something like: > ``` > + Filter: (ANY (base_tbl.a = $1) FROM SubPlan 1 (returns $1)) > + SubPlan 1 (returns $1) > ``` > ... arguably doesn't give much more information to the user comparing > to what we have now: > ``` > - Filter: (SubPlan 1) > - SubPlan 1 > ``` Yeah, I would probably not have thought to do this on my own; but we had an actual user request for it. I think arguably the main benefit is to confirm "yes, this is the sub-select you think it is". The main thing that's still missing compared to what is in the plan data structure is information about which Param is which. I think we have the subplan output Params relatively well covered through the expedient of listing them in the generated plan_name, but it's still not apparent to me how we could shoehorn subplan input Params into this (or whether it's worth doing). regards, tom lane
Re: the s_lock_stuck on perform_spin_delay
On Mon, Jan 22, 2024 at 11:58 AM Andy Fan wrote: > I get your point! Acquiring an already held spinlock in quickdie is > unlikely to happen, but since our existing infrastructure can handle it, > then there is no reason to bypass it. No, the existing infrastructure cannot handle that at all. -- Robert Haas EDB: http://www.enterprisedb.com
Re: XLog size reductions: smaller XLRec block header for PG17
On Mon, 22 Jan 2024 at 16:08, Aleksander Alekseev wrote: > > Hi, > > > I'm seeing that there has been no activity in this thread for nearly 4 > > months, I'm planning to close this in the current commitfest unless > > someone is planning to take it forward. > > I don't think that closing CF entries purely due to inactivity is a > good practice (neither something we did before) as long as there is > code, it applies, etc. There are a lot of patches and few people > working on them. Inactivity in a given thread doesn't necessarily > indicate lack of interest, more likely lack of resources. There are a lot of patches like this and there is no clear way to find out if someone wants to work on it or if they have lost interest in it. That is the reason, I thought to send out a mail so that the author/reviewer can reply and take it to the next state like ready for committer state. If the author/reviewer is not planning to work in this commitfest, but has plans to work in the next commitfest we can move this to the next commitfest. I don't see a better way to identify if the patch has interest or not. Regards, Vignesh
Re: the s_lock_stuck on perform_spin_delay
Robert Haas writes: > On Mon, Jan 22, 2024 at 2:22 AM Andy Fan wrote: >> I used sigismember(, SIGQUIT) to detect if a process is doing a >> quickdie, however this is bad not only because it doesn't work on >> Windows, but also it has too poor performance even it impacts on >> USE_ASSERT_CHECKING build only. In v8, I introduced a new global >> variable quickDieInProgress to handle this. > > OK, I like the split between 0001 and 0002. I still think 0001 has > cosmetic problems, but if some committer wants to take it forward, > they can decide what to do about that; you and I going back and forth > doesn't seem like the right approach to sorting that out. Whether or > not 0002 is adopted might affect what we do about the cosmetics in > 0001, too. Replacing ASSERT_NO_SPIN_LOCK with VerifyNoSpinLocksHeld or adding more comments to each call of VerifyNoSpinLocksHeld really makes me happy since they make things more cosmetic and practical. So I'd be absolutely willing to do more stuff like this. Thanks for such suggestions! Then I can't understand the left cosmetic problems... since you are saying it may related to 0002, I guess you are talking about the naming of START_SPIN_LOCK and END_SPIN_LOCK? > > 0003 seems ... unfortunate. It seems like an admission that 0001 is > wrong. Yes, that's what I was thinking. I doubted if I should merge 0003 to 0001 directly during this discussion, and finally I made it separate for easier dicussion. > Surely it *isn't* right to ignore the spinlock restrictions in > quickdie() in general. For example, we could self-deadlock if we try > to acquire a spinlock we already hold. If the problem here is merely > the call in errstart() then maybe we need to rethink that particular > call. If it goes any deeper than that, maybe we've got actual bugs we > need to fix. I get your point! Acquiring an already held spinlock in quickdie is unlikely to happen, but since our existing infrastructure can handle it, then there is no reason to bypass it. Since the problem here is just errstart, we can do a if(!quickDieInProgress) VerifyNoSpinLocksHeld(); in errstart only. Another place besides the errstart is the CHECK_FOR_INTERRUPTS in errfinish. I think we can add the same check for the VerifyNoSpinLocksHeld in CHECK_FOR_INTERRUPTS. > > +* It's likely to check the BlockSig to know if it is doing a quickdie > +* with sigismember, but it is too expensive in test, so introduce > +* quickDieInProgress to avoid that. > > This isn't very good English -- I realize that can sometimes be hard > -- but also -- I don't think it likely that a future hacker would > wonder why this isn't done that way. A static variable is normal for > PostgreSQL; checking the signal mask would be a completely novel > approach. So I think this comment is missing the mark topically. I was wrong to think sigismember is a syscall, but now I see it is just a function in glibc. Even I can't get the source code of it, I think it should just be some bit-operation based on the definition of __sigset_t. Another badness of sigismember is it is not avaiable in windows. It is still unclear to me why sigaddset in quickdie can compile in windows. (I have a sigismember version and get a linker error at windows). This should be a blocker for me to submit the next version of patch. > If > this patch is right at all, the comment here should focus on why > disabling these checks in quickdie() is necessary and appropriate, not > why it's coded to match everything else in the system. I agree, and I think the patch 0003 is not right at all:( -- Best Regards Andy Fan
Re: Add \syncpipeline command to pgbench
On 2024-Jan-22, Anthonin Bonnefoy wrote:
> 0001 introduces a new error when the end of a pgbench script is
> reached while there's still an ongoing pipeline.
Pushed, backpatched to 14. I reworded the error message to be
client %d aborted: end of script reached with pipeline open
I hope this is OK. I debated a half a dozen alternatives ("with open
pipeline", "without closing pipeline", "with unclosed pipeline" (???),
"leaving pipeline open") and decided this was the least ugly.
Thanks,
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
really, I see PHP as like a strange amalgamation of C, Perl, Shell
inflex: you know that "amalgam" means "mixture with mercury",
more or less, right?
i.e., "deadly poison"
Re: pg_stat_statements and "IN" conditions
Dmitry Dolgov <9erthali...@gmail.com> writes: >> On Mon, Jan 22, 2024 at 05:33:26PM +1100, Peter Smith wrote: >> Hi, This patch has a CF status of "Needs Review" [1], but it seems >> there was a CFbot test failure last time it was run [2]. Please have a >> look and post an updated version if necessary. >> >> == >> [1] https://commitfest.postgresql.org/46/2837/ >> [2] https://cirrus-ci.com/task/6688578378399744 > It's the same failing pipeline Vignesh C was talking above. I've fixed > the issue in the latest patch version, but looks like it wasn't picked > up yet (from what I understand, the latest build for this CF is 8 weeks > old). Please notice that at the moment, it's not being tested at all because of a patch-apply failure -- that's what the little triangular symbol means. The rest of the display concerns the test results from the last successfully-applied patch version. (Perhaps that isn't a particularly great UI design.) If you click on the triangle you find out == Applying patches on top of PostgreSQL commit ID b0f0a9432d0b6f53634a96715f2666f6d4ea25a1 === === applying patch ./v17-0001-Prevent-jumbling-of-every-element-in-ArrayExpr.patch patching file contrib/pg_stat_statements/Makefile Hunk #1 FAILED at 19. 1 out of 1 hunk FAILED -- saving rejects to file contrib/pg_stat_statements/Makefile.rej patching file contrib/pg_stat_statements/expected/merging.out patching file contrib/pg_stat_statements/meson.build ... regards, tom lane
Re: psql: Allow editing query results with \gedit
On Mon, Jan 22, 2024 at 8:06 AM Christoph Berg wrote: > Assuming a SELECT statement reading from a single table, it is quite an > effort to transform that statement to an UPDATE statement on that table, > perhaps to fix a typo that the user has spotted in the query result. > > Building off the other comments, I'd suggest trying to get rid of the intermediate JSOn format and also just focus on a single row at any given time. For an update the first argument to the metacommand could be the unique key value present in the previous result. The resultant UPDATE would just put that into the where clause and every other column in the result would be a SET clause column with the thing being set the current value, ready to be edited. DELETE would be similar but without the need for a SET clause. INSERT can produce a template INSERT (cols) VALUES ... command with some smarts regarding auto incrementing keys and placeholder values. David J.
Re: Support TZ format code in to_timestamp()
Daniel Gustafsson writes:
> On 22 Jan 2024, at 03:10, Tom Lane wrote:
> + while (len > 0)
> + {
> + const datetkn *tp = datebsearch(lowtoken, zoneabbrevtbl->abbrevs,
> + zoneabbrevtbl->numabbrevs);
> My immediate reaction was that we should stop at prefix lengths of two since I
> could only think of abbreviations of two or more. Googling and reading found
> that there are indeed one-letter timezones (Alpha, Bravo etc..). Not sure if
> it's worth mentioning that in the comment to help other readers who aren't
> neck
> deep in timezones?
The one I usually think of is "Z" for UTC; I wasn't actually aware
that there were any other single-letter abbrevs. But in any case
I don't see a reason for this code to be making such assumptions.
> + /* FALL THRU */
> Tiny nitpick, it looks a bit curious that we spell it FALL THRU here and "fall
> through" a few cases up in the same switch. While we are quite inconsistent
> across the tree, consistency within a file is preferrable (regardless of
> which).
Fair. I tend to shorten it, but I failed to notice that there was
nearby precedent for the other way.
regards, tom lane
Re: remaining sql/json patches
On 2024-Jan-18, Alvaro Herrera wrote: > > commands/explain.c (Hmm, I think this is a preexisting bug actually) > > > > 3893 18 : case T_TableFuncScan: > > 3894 18 : Assert(rte->rtekind == RTE_TABLEFUNC); > > 3895 18 : if (rte->tablefunc) > > 3896 0 : if (rte->tablefunc->functype == > > TFT_XMLTABLE) > > 3897 0 : objectname = "xmltable"; > > 3898 : else/* Must be > > TFT_JSON_TABLE */ > > 3899 0 : objectname = "json_table"; > > 3900 : else > > 3901 18 : objectname = NULL; > > 3902 18 : objecttag = "Table Function Name"; > > 3903 18 : break; > > Indeed I was completely wrong about this, and in order to gain coverage the only thing we needed was to add an EXPLAIN that uses the JSON format. I did that just now. I think your addition here works just fine. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Re: psql: Allow editing query results with \gedit
Pavel Stehule writes: > po 22. 1. 2024 v 16:06 odesílatel Christoph Berg napsal: >> This patch automates the tedious parts by opening the query result in a >> editor in JSON format, where the user can edit the data. On closing the >> editor, the JSON data is read back, and the differences are sent as >> UPDATE commands. New rows are INSERTed, and deleted rows are DELETEd. > Introduction of \gedit is interesting idea, but in this form it looks too > magic Yeah, I don't like it either --- it feels like something that belongs in an ETL tool not psql. The sheer size of the patch shows how far afield it is from anything that psql already does, necessitating writing tons of stuff that was not there before. The bits that try to parse the query to get necessary information seem particularly half-baked. > In the end I am not sure if I like it or dislike it. Looks dangerous. I can > imagine possible damage when some people will see vi first time and will > try to finish vi, but in this command, it will be transformed to executed > UPDATEs. Yup -- you'd certainly want some way of confirming that you actually want the changes applied. Our existing edit commands load the edited string back into the query buffer, where you can \r it if you don't want to run it. But I fear that the results of this operation would be too long for that to be workable. > More generating UPDATEs without knowledge of table structure > (knowledge of PK) can be issue (and possibly dangerous too), and you cannot > to recognize PK from result of SELECT (Everywhere PK is not "id" and it is > not one column). It does look like it's trying to find out the pkey from the system catalogs ... however, it's also accepting unique constraints without a lot of thought about the implications of NULLs. Even if you have a pkey, it's not exactly clear to me what should happen if the user changes the contents of a pkey field. That could be interpreted as either an INSERT or UPDATE, I think. Also, while I didn't read too closely, it's not clear to me how the code could reliably distinguish INSERT vs UPDATE vs DELETE cases --- surely we're not going to try to put a "diff" engine into this, and even if we did, diff is well known for producing somewhat surprising decisions about exactly which old lines match which new ones. That's part of the reason why I really don't like the idea that the deduced change commands will be summarily applied without the user even seeing them. regards, tom lane
Re: pg_stat_statements and "IN" conditions
> On Mon, Jan 22, 2024 at 05:33:26PM +1100, Peter Smith wrote: > 2024-01 Commitfest. > > Hi, This patch has a CF status of "Needs Review" [1], but it seems > there was a CFbot test failure last time it was run [2]. Please have a > look and post an updated version if necessary. > > == > [1] https://commitfest.postgresql.org/46/2837/ > [2] https://cirrus-ci.com/task/6688578378399744 It's the same failing pipeline Vignesh C was talking above. I've fixed the issue in the latest patch version, but looks like it wasn't picked up yet (from what I understand, the latest build for this CF is 8 weeks old).
Re: Adding facility for injection points (or probe points?) for more advanced tests
On 22/01/2024 06:38, Michael Paquier wrote:
0001~0004 have been now applied, and I'm marking the CF entry as
committed.
Woo-hoo!
I wrote the attached patch to enable injection points in the Cirrus CI
config, to run the injection tests I wrote for a GIN bug today [1]. But
that led to a crash in the asan-enabled build [2]. I didn't investigate
it yet.
[1]
https://www.postgresql.org/message-id/d8f0b068-0e6e-4b2c-8932-62507eb7e1c6%40iki.fi
[2] https://cirrus-ci.com/task/5242888636858368
--
Heikki Linnakangas
Neon (https://neon.tech)
From d641364bccec051761a9360453162929ef77ea3d Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas
Date: Mon, 22 Jan 2024 17:44:20 +0200
Subject: [PATCH 1/1] Enable injection points in cirrus CI
---
.cirrus.tasks.yml | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index e137769850d..0eacbbf3721 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -172,7 +172,7 @@ task:
su postgres <<-EOF
meson setup \
--buildtype=debug \
--Dcassert=true -Duuid=bsd -Dtcl_version=tcl86 -Ddtrace=auto \
+-Dcassert=true -Dinjection_points=true -Duuid=bsd -Dtcl_version=tcl86 -Ddtrace=auto \
-DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
-Dextra_lib_dirs=/usr/local/lib -Dextra_include_dirs=/usr/local/include/ \
build
@@ -327,7 +327,7 @@ task:
configure_script: |
su postgres <<-EOF
./configure \
---enable-cassert --enable-debug --enable-tap-tests \
+--enable-cassert --enable-injection-points --enable-debug --enable-tap-tests \
--enable-nls \
--with-segsize-blocks=6 \
\
@@ -357,7 +357,7 @@ task:
su postgres <<-EOF
meson setup \
--buildtype=debug \
--Dcassert=true \
+-Dcassert=true -Dinjection_points=true \
${LINUX_MESON_FEATURES} \
-DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
build
@@ -370,7 +370,7 @@ task:
export CC='ccache gcc -m32'
meson setup \
--buildtype=debug \
--Dcassert=true \
+-Dcassert=true -Dinjection_points=true \
${LINUX_MESON_FEATURES} \
-Dllvm=disabled \
--pkg-config-path /usr/lib/i386-linux-gnu/pkgconfig/ \
@@ -482,7 +482,7 @@ task:
--buildtype=debug \
-Dextra_include_dirs=/opt/local/include \
-Dextra_lib_dirs=/opt/local/lib \
- -Dcassert=true \
+ -Dcassert=true -Dinjection_points=true \
-Duuid=e2fs -Ddtrace=auto \
-DPG_TEST_EXTRA="$PG_TEST_EXTRA" \
build
@@ -556,7 +556,7 @@ task:
# Use /DEBUG:FASTLINK to avoid high memory usage during linking
configure_script: |
vcvarsall x64
-meson setup --backend ninja --buildtype debug -Dc_link_args=/DEBUG:FASTLINK -Dcassert=true -Db_pch=true -Dextra_lib_dirs=c:\openssl\1.1\lib -Dextra_include_dirs=c:\openssl\1.1\include -DTAR=%TAR% -DPG_TEST_EXTRA="%PG_TEST_EXTRA%" build
+meson setup --backend ninja --buildtype debug -Dc_link_args=/DEBUG:FASTLINK -Dcassert=true -Dinjection_points=true -Db_pch=true -Dextra_lib_dirs=c:\openssl\1.1\lib -Dextra_include_dirs=c:\openssl\1.1\include -DTAR=%TAR% -DPG_TEST_EXTRA="%PG_TEST_EXTRA%" build
build_script: |
vcvarsall x64
@@ -616,7 +616,7 @@ task:
# disable -Dnls as the number of files it creates cause a noticable slowdown
configure_script: |
-%BASH% -c "meson setup -Ddebug=true -Doptimization=g -Dcassert=true -Db_pch=true -Dnls=disabled -DTAR=%TAR% build"
+%BASH% -c "meson setup -Ddebug=true -Doptimization=g -Dcassert=true -Dinjection_points=true -Db_pch=true -Dnls=disabled -DTAR=%TAR% build"
build_script: |
%BASH% -c "ninja -C build"
--
2.39.2
Re: Add tuples_skipped to pg_stat_progress_copy
On 2024-01-17 14:47, Masahiko Sawada wrote:
On Wed, Jan 17, 2024 at 2:22 PM torikoshia
wrote:
Hi,
132de9968840c introduced SAVE_ERROR_TO option to COPY and enabled to
skip malformed data, but there is no way to watch the number of
skipped
rows during COPY.
Attached patch adds tuples_skipped to pg_stat_progress_copy, which
counts the number of skipped tuples because source data is malformed.
If SAVE_ERROR_TO is not specified, this column remains zero.
The advantage would be that users can quickly notice and stop COPYing
when there is a larger amount of skipped data than expected, for
example.
As described in commit log, it is expected to add more choices for
SAVE_ERROR_TO like 'log' and using such options may enable us to know
the number of skipped tuples during COPY, but exposed in
pg_stat_progress_copy would be easier to monitor.
What do you think?
+1
The patch is pretty simple. Here is a comment:
+ (if SAVE_ERROR_TO is specified, otherwise
zero).
+
+
To be precise, this counter only advances when a value other than
'ERROR' is specified to SAVE_ERROR_TO option.
Thanks for your comment and review!
Updated the patch according to your comment and option name change by
b725b7eec.
BTW, based on this patch, I think we can add another option which
specifies the maximum tolerable number of malformed rows.
I remember this was discussed in [1], and feel it would be useful when
loading 'dirty' data but there is a limit to how dirty it can be.
Attached 0002 is WIP patch for this(I haven't added doc yet).
This may be better discussed in another thread, but any comments(e.g.
necessity of this option, option name) are welcome.
[1]
https://www.postgresql.org/message-id/752672.1699474336%40sss.pgh.pa.us
--
Regards,
--
Atsushi Torikoshi
NTT DATA Group CorporationFrom 571ada768bdb68a31f295cbcb28f4348f253989d Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi
Date: Mon, 22 Jan 2024 23:57:24 +0900
Subject: [PATCH v2 1/2] Add tuples_skipped to pg_stat_progress_copy
132de9968840c enabled COPY to skip malformed data, but there is no way to watch
the number of skipped rows during COPY.
This patch adds tuples_skipped to pg_stat_progress_copy, which counts the
number of skipped tuple because source data is malformed.
This column only advances when a value other than stop is specified to ON_ERROR.
Needs catalog bump.
---
doc/src/sgml/monitoring.sgml | 11 +++
src/backend/catalog/system_views.sql | 3 ++-
src/backend/commands/copyfrom.c | 5 +
src/include/commands/progress.h | 1 +
src/test/regress/expected/rules.out | 3 ++-
5 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 6e74138a69..cfc13b3580 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -5780,6 +5780,17 @@ FROM pg_stat_get_backend_idset() AS backendid;
WHERE clause of the COPY command.
+
+
+
+ tuples_skipped bigint
+
+
+ Number of tuples skipped because they contain malformed data.
+ This counter only advances when a value other than
+ stop is specified to ON_ERROR.
+
+
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index e43e36f5ac..6288270e2b 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1318,7 +1318,8 @@ CREATE VIEW pg_stat_progress_copy AS
S.param1 AS bytes_processed,
S.param2 AS bytes_total,
S.param3 AS tuples_processed,
-S.param4 AS tuples_excluded
+S.param4 AS tuples_excluded,
+S.param7 AS tuples_skipped
FROM pg_stat_get_progress_info('COPY') AS S
LEFT JOIN pg_database D ON S.datid = D.oid;
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 173a736ad5..8ab3777664 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -650,6 +650,7 @@ CopyFrom(CopyFromState cstate)
CopyMultiInsertInfo multiInsertInfo = {0}; /* pacify compiler */
int64 processed = 0;
int64 excluded = 0;
+ int64 skipped = 0;
bool has_before_insert_row_trig;
bool has_instead_insert_row_trig;
bool leafpart_use_multi_insert = false;
@@ -1012,6 +1013,10 @@ CopyFrom(CopyFromState cstate)
*/
cstate->escontext->error_occurred = false;
+ /* Report that this tuple was skipped by the ON_ERROR clause */
+ pgstat_progress_update_param(PROGRESS_COPY_TUPLES_SKIPPED,
+ ++skipped);
+
continue;
}
diff --git a/src/include/commands/progress.h b/src/include/commands/progress.h
index a458c8c50a..73afa77a9c 100644
--- a/src/include/commands/progress.h
+++ b/src/include/commands/progress.h
@@ -142,6 +142,7 @@
#define PROGRESS_COPY_TUPLES_EXCLUDED 3
#define PROGRESS_COPY_COMMAND 4
#define PROGRESS_COPY_TYPE 5
+#define PROGRESS_COPY_TUPLES_SKIPPED 6
/*
Re: Support TZ format code in to_timestamp()
> On 22 Jan 2024, at 03:10, Tom Lane wrote:
> I still think it would be a good idea, but I can't deny the lack
> of other interest in it. Unless someone steps up to review,
> let's close it.
Since I had this on my (ever-growing) TODO I re-prioritized today and took a
look at it since I think it's something we should support.
Nothing really sticks out and I was unable to poke any holes so I don't have
too much more to offer than a LGTM.
+ while (len > 0)
+ {
+ const datetkn *tp = datebsearch(lowtoken, zoneabbrevtbl->abbrevs,
+ zoneabbrevtbl->numabbrevs);
My immediate reaction was that we should stop at prefix lengths of two since I
could only think of abbreviations of two or more. Googling and reading found
that there are indeed one-letter timezones (Alpha, Bravo etc..). Not sure if
it's worth mentioning that in the comment to help other readers who aren't neck
deep in timezones?
+ /* FALL THRU */
Tiny nitpick, it looks a bit curious that we spell it FALL THRU here and "fall
through" a few cases up in the same switch. While we are quite inconsistent
across the tree, consistency within a file is preferrable (regardless of
which).
--
Daniel Gustafsson
Re: remaining sql/json patches
On Mon, Jan 22, 2024 at 10:28 PM Amit Langote wrote:
>
> > based on v35.
> > Now I only applied from 0001 to 0007.
> > For {DEFAULT expression ON EMPTY} | {DEFAULT expression ON ERROR}
> > restrict DEFAULT expression be either Const node or FuncExpr node.
> > so these 3 SQL/JSON functions can be used in the btree expression index.
>
> I'm not really excited about adding these restrictions into the
> transformJsonFuncExpr() path. Index or any other code that wants to
> put restrictions already have those in place, no need to add them
> here. Moreover, by adding these restrictions, we might end up
> preventing users from doing useful things with this like specify
> column references. If there are semantic issues with allowing that,
> we should discuss them.
>
after applying v36.
The following index creation and query operation works. I am not 100%
sure about these cases.
just want confirmation, sorry for bothering you
drop table t;
create table t(a jsonb, b int);
insert into t select '{"hello":11}',1;
insert into t select '{"hello":12}',2;
CREATE INDEX t_idx2 ON t (JSON_query(a, '$.hello1' RETURNING int
default b + random() on error));
CREATE INDEX t_idx3 ON t (JSON_query(a, '$.hello1' RETURNING int
default random()::int on error));
SELECT JSON_query(a, '$.hello1' RETURNING int default ret_setint() on
error) from t;
SELECT JSON_query(a, '$.hello1' RETURNING int default sum(b) over()
on error) from t;
SELECT JSON_query(a, '$.hello1' RETURNING int default sum(b) on
error) from t group by a;
but the following cases will fail related to index and default expression.
create table zz(a int, b int);
CREATE INDEX zz_idx1 ON zz ( (b + random()::int));
create table (a int, b int default ret_setint());
create table (a int, b int default sum(b) over());
Re: psql: Allow editing query results with \gedit
Hi
po 22. 1. 2024 v 16:06 odesílatel Christoph Berg napsal:
> Assuming a SELECT statement reading from a single table, it is quite an
> effort to transform that statement to an UPDATE statement on that table,
> perhaps to fix a typo that the user has spotted in the query result.
>
> First, the general syntax is not the same with the order of syntax
> elements changed. Then the row in question needs to be pinned down by
> the primary key, requiring cut-and-paste of the PK columns. Furthermore,
> the value to be updated needs to be put into the command, with proper
> quoting. If the original value spans multiple line, copy-and-pasting it
> for editing is especially tedious.
>
> Suppose the following query where we spot a typo in the 2nd message:
>
> =# select id, language, message from messages where language = 'en';
> id | language | message
> 1 | en | Good morning
> 2 | en | Hello warld
>
> The query needs to be transformed into this update:
>
> =# update messages set message = 'Hello world' where id = 2;
>
> This patch automates the tedious parts by opening the query result in a
> editor in JSON format, where the user can edit the data. On closing the
> editor, the JSON data is read back, and the differences are sent as
> UPDATE commands. New rows are INSERTed, and deleted rows are DELETEd.
>
> =# select id, language, message from messages where language = 'en' \gedit
>
> An editor opens:
> [
> { "id": 1, "language": "en", "message": "Good morning" },
> { "id": 2, "language": "en", "message": "Hello warld" }
> ]
>
> Let's fix the typo and save the file:
> [
> { "id": 1, "language": "en", "message": "Good morning" },
> { "id": 2, "language": "en", "message": "Hello world" }
> ]
> UPDATE messages SET message = 'Hello world' WHERE id = '2';
> UPDATE 1
>
> In this example, typing "WHERE id = 2" would not be too hard, but the
> primary key might be a composite key, with complex non-numeric values.
> This is supported as well.
>
> If expanded mode (\x) is enabled, \gedit will use the expanded JSON
> format, best suitable for long values.
>
>
> This patch requires the "psql JSON output format" patch.
>
Introduction of \gedit is interesting idea, but in this form it looks too
magic
a) why the data are in JSON format, that is not native for psql (minimally
now)
b) the implicit transformation to UPDATEs and the next evaluation can be
pretty dangerous.
The concept of proposed feature is interesting, but the name \gedit is too
generic, maybe too less descriptive for this purpose
Maybe \geditupdates can be better - but still it can be dangerous and slow
(without limits)
In the end I am not sure if I like it or dislike it. Looks dangerous. I can
imagine possible damage when some people will see vi first time and will
try to finish vi, but in this command, it will be transformed to executed
UPDATEs. More generating UPDATEs without knowledge of table structure
(knowledge of PK) can be issue (and possibly dangerous too), and you cannot
to recognize PK from result of SELECT (Everywhere PK is not "id" and it is
not one column).
Regards
Pavel
> Christoph
>
Re: the s_lock_stuck on perform_spin_delay
On Mon, Jan 22, 2024 at 2:22 AM Andy Fan wrote: > I used sigismember(, SIGQUIT) to detect if a process is doing a > quickdie, however this is bad not only because it doesn't work on > Windows, but also it has too poor performance even it impacts on > USE_ASSERT_CHECKING build only. In v8, I introduced a new global > variable quickDieInProgress to handle this. OK, I like the split between 0001 and 0002. I still think 0001 has cosmetic problems, but if some committer wants to take it forward, they can decide what to do about that; you and I going back and forth doesn't seem like the right approach to sorting that out. Whether or not 0002 is adopted might affect what we do about the cosmetics in 0001, too. 0003 seems ... unfortunate. It seems like an admission that 0001 is wrong. Surely it *isn't* right to ignore the spinlock restrictions in quickdie() in general. For example, we could self-deadlock if we try to acquire a spinlock we already hold. If the problem here is merely the call in errstart() then maybe we need to rethink that particular call. If it goes any deeper than that, maybe we've got actual bugs we need to fix. +* It's likely to check the BlockSig to know if it is doing a quickdie +* with sigismember, but it is too expensive in test, so introduce +* quickDieInProgress to avoid that. This isn't very good English -- I realize that can sometimes be hard -- but also -- I don't think it likely that a future hacker would wonder why this isn't done that way. A static variable is normal for PostgreSQL; checking the signal mask would be a completely novel approach. So I think this comment is missing the mark topically. If this patch is right at all, the comment here should focus on why disabling these checks in quickdie() is necessary and appropriate, not why it's coded to match everything else in the system. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Support TZ format code in to_timestamp()
Hi,
> > Hi, this patch was marked in CF as "Needs Review" [1], but there has
> > been no activity on this thread for 7+ months.
> > If nothing more is planned for this thread then it will be closed
> > ("Returned with feedback") at the end of this CF.
>
> I still think it would be a good idea, but I can't deny the lack
> of other interest in it. Unless someone steps up to review,
> let's close it.
I agree that it would be a good idea, and again I would like to
condemn the approach "since no one reviews it we are going to reject
it". A friendly reminder like "hey, this patch was waiting long
enough, maybe someone could take a look" would be more appropriate
IMO. I remember during previous commitfests some CF managers created a
list of patches that could use more attention. That was useful.
I will review the patch, but probably only tomorrow.
--
Best regards,
Aleksander Alekseev
Re: psql JSON output format
Re: Laurenz Albe > > But I do think it has positive > > value. If we produce output that could be ingested back into PG later > > with the right tool, that leaves the door open for someone to build > > the tool later even if we don't have it today. If we produce output > > that loses information, no tool built later can make up for the loss. > I am all for losing as little information as possible, but I think > that this discussion is going off on a tangent. After all, we are not > talking about a data export tool here, we are talking about psql. I've just posted the other patch where I need the JSON format: https://www.postgresql.org/message-id/flat/Za6EfXeewwLRS_fs%40msg.df7cb.de There, I need to be able to read back the query output into psql, while at the same time being human-readable so the user can sanely edit the data in an editor. The default "aligned" format is only human-readable, while CSV is mostly only machine-readable. JSON is the best option between the two, I think. What I did now in v3 of this patch is to print boolean and numeric values (ints, floats, numeric) without quotes, while adding the quotes back to json. This solves the NULL vs 'null'::json problem. > I don't see anybody complain that float8 values lose precision in > the default aligned format, or that empty strings and NULL values > look the same in aligned format. Why do the goalposts move for the > JSON output format? I don't think psql output should be considered > a form of backup. Fwiw, not quoting numbers in JSON won't have any of these problems if the JSON reader just passes the strings read through. (Which PG's JSON parser does.) > Can we get consensus that SQL NULL columns should be omitted from the > output, and the rest left as it currently is? I think that would be an interesting option for a JSON export format. The psql JSON format is more for human inspection, where omitting the columns might create confusion. (We could make it a pset parameter of the format, but I think the default should be to show NULL columns.) Christoph >From 2b575846fee609237256fe8eaf38fd45005fe8da Mon Sep 17 00:00:00 2001 From: Christoph Berg Date: Fri, 8 Sep 2023 15:59:29 +0200 Subject: [PATCH] Add JSON output format to psql Query results are formatted as an array of JSON objects. In non-expanded mode, one object per line is printed, and in expanded mode, one key-value pair. Use `\pset format json` to enable, or run `psql --json` or `psql -J`. NULL values are printed as `null`, independently from the psql null setting. Numeric values are printed in plain, booleans as true/false, and other values are printed as quoted strings. --- doc/src/sgml/ref/psql-ref.sgml | 26 - src/bin/psql/command.c | 6 +- src/bin/psql/help.c| 1 + src/bin/psql/startup.c | 6 +- src/bin/psql/tab-complete.c| 2 +- src/fe_utils/print.c | 182 - src/include/fe_utils/print.h | 6 +- src/test/regress/expected/psql.out | 79 + src/test/regress/sql/psql.sql | 28 + 9 files changed, 325 insertions(+), 11 deletions(-) diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index cc7d797159..f1f7eda082 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -274,6 +274,17 @@ EOF + + -J + --json + + + Switches to JSON output mode. This is + equivalent to \pset format json. + + + + -l --list @@ -2956,6 +2967,7 @@ lo_import 152801 asciidoc, csv, html, + json, latex, latex-longtable, troff-ms, unaligned, or wrapped. @@ -2972,7 +2984,7 @@ lo_import 152801 in by other programs, for example, tab-separated or comma-separated format. However, the field separator character is not treated specially if it appears in a column's value; - so CSV format may be better suited for such + the CSV or JSON formats may be better suited for such purposes. @@ -2997,6 +3009,18 @@ lo_import 152801 \pset csv_fieldsep. + json format + + JSON format + in psql + + writes output in JSON format, described in + https://www.ietf.org/rfc/rfc4627.txt;>RFC 4627. + The result is an array of JSON objects. In non-expanded mode, one + object per line is printed, while in expanded mode, each key-value + pair is a separate line. + + wrapped format is like aligned but wraps wide data values across lines to make the output fit in the target column width. The target width is determined as described under diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index 5c906e4806..824900819b 100644 ---
