Re: isolationtester - allow a session specific connection string
On Mon, Dec 19, 2022 at 5:35 PM Peter Smith wrote:
>
> On Mon, Dec 19, 2022 at 5:04 PM Tom Lane wrote:
> >
> > Peter Smith writes:
> > > My patch/idea makes a small change to the isolationtester spec
> > > grammar. Now each session can optionally specify its own connection
> > > string. When specified, this will override any connection string for
> > > that session that would otherwise have been used. This is the only
> > > change.
> >
> > Surely this cannot work, because isolationtester only runs one
> > monitoring session. How will it detect wait conditions for
> > sessions connected to some other postmaster?
> >
>
> You are right - probably it can't work in a generic sense. But if the
> "controller session" (internal session 0) is also configured to use
> the same conninfo as all my "publisher" sessions (the current patch
> can't do this but it seems only a small change) then all of the
> publisher-side sessions will be monitored like they ought to be --
> which is all I really needed I think.
>
PSA v2 of this patch. Now the conninfo can be specified at the *.spec
file global scope. This will set the connection string for the
"controller", and this will be used by every other session unless they
too specify a conninfo. For example,
==
# Set the isolationtester controller's conninfo. User sessions will also use
# this unless they specify otherwise.
conninfo "host=localhost port=7651"
# Publisher node
session ps1
setup
{
TRUNCATE TABLE tbl;
}
step ps1_ins{ INSERT INTO tbl VALUES (111); }
step ps1_sel{ SELECT * FROM tbl ORDER BY id; }
step ps1_begin { BEGIN; }
step ps1_commit { COMMIT; }
step ps1_rollback { ROLLBACK; }
session ps2
step ps2_ins{ INSERT INTO tbl VALUES (222); }
step ps2_sel{ SELECT * FROM tbl ORDER BY id; }
step ps2_begin { BEGIN; }
step ps2_commit { COMMIT; }
step ps2_rollback { ROLLBACK; }
#
# Subscriber node
#
session sub
conninfo "host=localhost port=7652"
setup
{
TRUNCATE TABLE tbl;
}
step sub_sleep { SELECT pg_sleep(3); }
step sub_sel{ SELECT * FROM tbl ORDER BY id; }
...
==
The above spec file gives:
==
Parsed test spec with 3 sessions
control connection conninfo 'host=localhost port=7651'
ps1 conninfo 'host=localhost port=7651'
ps2 conninfo 'host=localhost port=7651'
sub conninfo 'host=localhost port=7652'
WARNING: session sub is not using same connection as the controller
...
==
In this way, IIUC the isolationtester's session locking mechanism can
work OK at least for all of my "publishing" sessions.
--
Kind Regards,
Peter Smith
Fujitsu Australia
v2-0001-isolationtester-allow-conninfo-to-be-specified.patch
Description: Binary data
Re: Add LSN along with offset to error messages reported for WAL file read/write/validate header failures
On Tue, Dec 06, 2022 at 04:27:50PM +0900, Kyotaro Horiguchi wrote: > At Mon, 5 Dec 2022 10:03:55 +0900, Michael Paquier > wrote in >> Hence I would tend to let XLogFromFileName do the job, while having a >> SQL function that is just a thin wrapper around it that returns the >> segment TLI and its number, leaving the offset out of the equation as >> well as this new XLogIdFromFileName(). > > I don't think it could be problematic that the SQL-callable function > returns a bogus result for a wrong WAL filename in the correct > format. Specifically, I think that the function may return (0/0,0) for > "" since that behavior is completely > harmless. If we don't check logid, XLogFromFileName fits instead. Yeah, I really don't think that it is a big deal either: XLogIdFromFileName() just translates what it receives from the caller for the TLI and the segment number. > (If we assume that the file names are typed in letter-by-letter, I > rather prefer to allow lower-case letters:p) Yep, makes sense to enforce a compatible WAL segment name if we can. -- Michael signature.asc Description: PGP signature
Re: pg_upgrade allows itself to be run twice
On Fri, Dec 16, 2022 at 07:38:09AM -0600, Justin Pryzby wrote:
> However, setting FirstNormalOid in initdb itself (rather than in the
> next invocation of postgres, if it isn't in single-user-mode) was the
> mechanism by which to avoid the original problem that pg_upgrade can be
> run twice, if there are no non-system relations. That still seems
> desirable to fix somehow.
+ if (new_cluster.controldata.chkpnt_nxtoid != FirstNormalObjectId)
+ pg_fatal("New cluster is not pristine: OIDs have been assigned
since initdb (%u != %u)\n",
+ new_cluster.controldata.chkpnt_nxtoid,
FirstNormalObjectId);
On wraparound FirstNormalObjectId would be the first value we use for
nextOid. Okay, that's very unlikely going to happen, still, strictly
speaking, that could be incorrect.
>> I think this would be worth addressing nonetheless, for robustness. For
>> comparison, "cp" and "mv" will error if you give source and destination that
>> are the same file.
>
> I addressed this in 002.
+ if (strcmp(make_absolute_path(old_cluster.pgdata),
+ make_absolute_path(new_cluster.pgdata)) == 0)
+ pg_fatal("cannot upgrade a cluster on top of itself");
Shouldn't this be done after adjust_data_dir(), which is the point
where we'll know the actual data folders we are working on by querying
postgres -C data_directory?
--
Michael
signature.asc
Description: PGP signature
Re: isolationtester - allow a session specific connection string
On Mon, Dec 19, 2022 at 5:04 PM Tom Lane wrote: > > Peter Smith writes: > > My patch/idea makes a small change to the isolationtester spec > > grammar. Now each session can optionally specify its own connection > > string. When specified, this will override any connection string for > > that session that would otherwise have been used. This is the only > > change. > > Surely this cannot work, because isolationtester only runs one > monitoring session. How will it detect wait conditions for > sessions connected to some other postmaster? > You are right - probably it can't work in a generic sense. But if the "controller session" (internal session 0) is also configured to use the same conninfo as all my "publisher" sessions (the current patch can't do this but it seems only a small change) then all of the publisher-side sessions will be monitored like they ought to be -- which is all I really needed I think. -- Kind Regards, Peter Smith Fujitsu Australia
Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL
On Thu, Dec 15, 2022 at 05:17:46PM -0600, David Christensen wrote:
> On Thu, Dec 15, 2022 at 12:36 AM Michael Paquier wrote:
> This v10 should incorporate your feedback as well as Bharath's.
Thanks for the new version. I have minor comments.
>> It seems to me that you could allow things to work even if the
>> directory exists and is empty. See for example
>> verify_dir_is_empty_or_create() in pg_basebackup.c.
>
> The `pg_mkdir_p()` supports an existing directory (and I don't think
> we want to require it to be empty first), so this only errors when it
> can't create a directory for some reason.
Sure, but things can also be made so as we don't fail if the directory
exists and is empty? This would be more consistent with the base
directories created by pg_basebackup and initdb.
>> +$node->safe_psql('postgres', <> +SELECT 'init' FROM
>> pg_create_physical_replication_slot('regress_pg_waldump_slot', true, false);
>> +CREATE TABLE test_table AS SELECT generate_series(1,100) a;
>> +CHECKPOINT; -- required to force FPI for next writes
>> +UPDATE test_table SET a = a + 1;
>> Using an EOF to execute a multi-line query would be a first. Couldn't
>> you use the same thing as anywhere else? 009_twophase.pl just to
>> mention one. (Mentioned by Bharath upthread, where he asked for an
>> extra opinion so here it is.)
>
> Fair enough, while idiomatic perl to me, not a hill to die on;
> converted to a standard multiline string.
By the way, knowing that we have an option called --fullpage, could be
be better to use --save-fullpage for the option name?
+ OPF = fopen(filename, PG_BINARY_W);
+ if (!OPF)
+ pg_fatal("couldn't open file for output: %s", filename);
[..]
+ if (fwrite(page, BLCKSZ, 1, OPF) != 1)
+ pg_fatal("couldn't write out complete full page image to file: %s",
filename);
These should more more generic, as of "could not open file \"%s\"" and
"could not write file \"%s\"" as the file name provides all the
information about what this writes.
--
Michael
signature.asc
Description: PGP signature
appendBinaryStringInfo stuff
I found a couple of adjacent weird things:
There are a bunch of places in the json code that use
appendBinaryStringInfo() where appendStringInfoString() could be used, e.g.,
appendBinaryStringInfo(buf, ".size()", 7);
Is there a reason for this? Are we that stretched for performance? I
find this kind of code very fragile.
Also, the argument type of appendBinaryStringInfo() is char *. There is
some code that uses this function to assemble some kind of packed binary
layout, which requires a bunch of casts because of this. I think
functions taking binary data plus length should take void * instead,
like memcpy() for example.
Attached are two patches that illustrate these issues and show proposed
changes.From 15b103edec2f89a63596d112fd62cfc2367576bf Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 19 Dec 2022 07:01:27 +0100
Subject: [PATCH 1/2] Use appendStringInfoString instead of
appendBinaryStringInfo where possible
---
src/backend/utils/adt/jsonb.c| 10
src/backend/utils/adt/jsonpath.c | 42
2 files changed, 26 insertions(+), 26 deletions(-)
diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index 7c1e5e6144..c153c87ba6 100644
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -361,7 +361,7 @@ jsonb_put_escaped_value(StringInfo out, JsonbValue
*scalarVal)
switch (scalarVal->type)
{
case jbvNull:
- appendBinaryStringInfo(out, "null", 4);
+ appendStringInfoString(out, "null");
break;
case jbvString:
escape_json(out, pnstrdup(scalarVal->val.string.val,
scalarVal->val.string.len));
@@ -373,9 +373,9 @@ jsonb_put_escaped_value(StringInfo out, JsonbValue
*scalarVal)
break;
case jbvBool:
if (scalarVal->val.boolean)
- appendBinaryStringInfo(out, "true", 4);
+ appendStringInfoString(out, "true");
else
- appendBinaryStringInfo(out, "false", 5);
+ appendStringInfoString(out, "false");
break;
default:
elog(ERROR, "unknown jsonb scalar type");
@@ -565,7 +565,7 @@ JsonbToCStringWorker(StringInfo out, JsonbContainer *in,
int estimated_len, bool
/* json rules guarantee this is a string */
jsonb_put_escaped_value(out, &v);
- appendBinaryStringInfo(out, ": ", 2);
+ appendStringInfoString(out, ": ");
type = JsonbIteratorNext(&it, &v, false);
if (type == WJB_VALUE)
@@ -630,7 +630,7 @@ add_indent(StringInfo out, bool indent, int level)
appendStringInfoCharMacro(out, '\n');
for (i = 0; i < level; i++)
- appendBinaryStringInfo(out, "", 4);
+ appendStringInfoString(out, "");
}
}
diff --git a/src/backend/utils/adt/jsonpath.c b/src/backend/utils/adt/jsonpath.c
index 91af030095..a39eab9c20 100644
--- a/src/backend/utils/adt/jsonpath.c
+++ b/src/backend/utils/adt/jsonpath.c
@@ -213,7 +213,7 @@ jsonPathToCstring(StringInfo out, JsonPath *in, int
estimated_len)
enlargeStringInfo(out, estimated_len);
if (!(in->header & JSONPATH_LAX))
- appendBinaryStringInfo(out, "strict ", 7);
+ appendStringInfoString(out, "strict ");
jspInit(&v, in);
printJsonPathItem(out, &v, false, true);
@@ -510,9 +510,9 @@ printJsonPathItem(StringInfo buf, JsonPathItem *v, bool
inKey,
break;
case jpiBool:
if (jspGetBool(v))
- appendBinaryStringInfo(buf, "true", 4);
+ appendStringInfoString(buf, "true");
else
- appendBinaryStringInfo(buf, "false", 5);
+ appendStringInfoString(buf, "false");
break;
case jpiAnd:
case jpiOr:
@@ -553,13 +553,13 @@ printJsonPathItem(StringInfo buf, JsonPathItem *v, bool
inKey,
operationPriority(elem.type) <=
operationPriority(v->type));
- appendBinaryStringInfo(buf, " like_regex ", 12);
+ appendStringInfoString(buf, " like_regex ");
escape_json(buf, v->content.like_regex.pattern);
if (v->content.like_regex.flags)
{
- appendBina
Re: isolationtester - allow a session specific connection string
Peter Smith writes: > My patch/idea makes a small change to the isolationtester spec > grammar. Now each session can optionally specify its own connection > string. When specified, this will override any connection string for > that session that would otherwise have been used. This is the only > change. Surely this cannot work, because isolationtester only runs one monitoring session. How will it detect wait conditions for sessions connected to some other postmaster? regards, tom lane
isolationtester - allow a session specific connection string
Hi hackers.
I wanted some way to test overlapping transactions from different
publisher sessions so I could better test the logical replication
"parallel apply" feature being developed in another thread [1]. AFAIK
currently there is no way to do this kind of test except manually
(e.g. using separate psql sessions).
Meanwhile, using the isolationtester spec tests [2] it is already
possible to test overlapping transactions on multiple sessions. So
isolationtester already does almost everything I wanted except that it
currently only knows about a single connection string (either default
or passed as argument) which every one of the "spec sessions" uses. In
contrast, for my pub/sub testing, I wanted multiple servers.
The test_decoding tests [3] have specs a bit similar to this - the
difference is that I want to observe subscriber-side apply workers
execute and actually do something.
~
My patch/idea makes a small change to the isolationtester spec
grammar. Now each session can optionally specify its own connection
string. When specified, this will override any connection string for
that session that would otherwise have been used. This is the only
change.
With this change now it is possible to write spec test code like
below. Here I have 2 publisher sessions and 1 subscriber session, with
my new session ‘conninfo’ specified appropriately for each session
==
# This test assumes there is already setup as follows:
#
# PG server for publisher (running on port 7651)
# - has TABLE tbl
# - has PUBLICATION pub1
#
# PG server for subscriber (running on port 7652)
# - has TABLE tbl
# - has SUBSCRIPTION sub1 subscribing to pub1
#
# Publisher node
session ps1
conninfo "host=localhost port=7651"
setup
{
TRUNCATE TABLE tbl;
}
step ps1_ins{ INSERT INTO tbl VALUES (111); }
step ps1_sel{ SELECT * FROM tbl ORDER BY id; }
step ps1_begin { BEGIN; }
step ps1_commit { COMMIT; }
step ps1_rollback { ROLLBACK; }
session ps2
conninfo "host=localhost port=7651"
step ps2_ins{ INSERT INTO tbl VALUES (222); }
step ps2_sel{ SELECT * FROM tbl ORDER BY id; }
step ps2_begin { BEGIN; }
step ps2_commit { COMMIT; }
step ps2_rollback { ROLLBACK; }
#
# Subscriber node
#
session sub
conninfo "host=localhost port=7652"
setup
{
TRUNCATE TABLE tbl;
}
step sub_sleep { SELECT pg_sleep(3); }
step sub_sel{ SELECT * FROM tbl ORDER BY id; }
###
# Tests
###
# overlapping tx commits
permutation ps1_begin ps1_ins ps2_begin ps2_ins ps2_commit ps1_commit
sub_sleep sub_sel
permutation ps1_begin ps1_ins ps2_begin ps2_ins ps1_commit ps2_commit
sub_sleep sub_sel
==
Because there is still some external setup needed to make the 2
servers (with their own configurations and publication/subscription)
this kind of spec test can't be added to the 'isolation_schedule'
file. But even so, it seems to be working OK, so I think this
isolationtester enhancement can be an efficient way to write and run
some difficult pub/sub regression tests without having to test
everything entirely manually each time.
Thoughts?
~~
PSA
v1-0001 - This is the enhancement to add 'conninfo' to the isloationtester.
v1-0002 - An example of how 'conninfo' can be used (requires external setup)
test_init.sh - this is my external setup script pre-requisite for the
pub-sub.spec in v1-0002
--
[1] parallel apply thread -
https://www.postgresql.org/message-id/flat/CAA4eK1%2BwyN6zpaHUkCLorEWNx75MG0xhMwcFhvjqm2KURZEAGw%40mail.gmail.com
[2] isolation tests -
https://github.com/postgres/postgres/blob/master/src/test/isolation/README
[3] test_decoding spec tests -
https://github.com/postgres/postgres/tree/master/contrib/test_decoding/specs
Kind Regards,
Peter Smith
Fujitsu Australia
#!/bin/bash
port_PUB=7651
port_SUB=7652
function cleanup_nodes()
{
echo 'Clean up'
pg_ctl stop -D data_PUB
pg_ctl stop -D data_SUB
rm -r data_PUB data_SUB *log
}
function setup_nodes()
{
echo 'Set up'
# Create publisher/subscriber
initdb -D data_PUB
initdb -D data_SUB
cat << EOF >> data_PUB/postgresql.conf
wal_level = logical
port = $port_PUB
max_replication_slots=40
log_min_messages = debug1
force_stream_mode=true
autovacuum = off
EOF
cat << EOF >> data_SUB/postgresql.conf
port = $port_SUB
max_logical_replication_workers=100
max_replication_slots=40
max_parallel_apply_workers_per_subscription=99
log_min_messages = debug1
autovacuum = off
EOF
pg_ctl -D data_PUB start -w -l PUB.log
pg_ctl -D data_SUB start -w -l SUB.log
# Create publication/subscription
psql -p $port_PUB << EOF
CREATE TABLE tbl(id int);
CREATE PUBLICATION pub1 FOR TABLE tbl;
EOF
psql -p $port_SUB << EOF
CREATE TABLE tbl(id int);
CREATE SUBSCRIPTION sub1
CONNECTION 'port=$port_PUB'
PUBLICATION pub1
WITH (copy_data = off, streaming = parallel);
EOF
}
# =
Simplifications for error messages related to compression
Hi all,
While looking at a different patch, I have noticed that the error
messages produced by pg_basebackup and pg_dump are a bit inconsistent
with the other others. Why not switching all of them as of the
attached? This reduces the overall translation effort, using more:
"this build does not support compression with %s"
Thoughts or objections?
--
Michael
diff --git a/src/bin/pg_basebackup/bbstreamer_gzip.c b/src/bin/pg_basebackup/bbstreamer_gzip.c
index c3455ffbdd..ad1b79e6f2 100644
--- a/src/bin/pg_basebackup/bbstreamer_gzip.c
+++ b/src/bin/pg_basebackup/bbstreamer_gzip.c
@@ -113,7 +113,7 @@ bbstreamer_gzip_writer_new(char *pathname, FILE *file,
return &streamer->base;
#else
- pg_fatal("this build does not support gzip compression");
+ pg_fatal("this build does not support compression with %s", "gzip");
return NULL;/* keep compiler quiet */
#endif
}
@@ -246,7 +246,7 @@ bbstreamer_gzip_decompressor_new(bbstreamer *next)
return &streamer->base;
#else
- pg_fatal("this build does not support gzip compression");
+ pg_fatal("this build does not support compression with %s", "gzip");
return NULL;/* keep compiler quiet */
#endif
}
diff --git a/src/bin/pg_basebackup/bbstreamer_lz4.c b/src/bin/pg_basebackup/bbstreamer_lz4.c
index ed2aa01305..14282c4833 100644
--- a/src/bin/pg_basebackup/bbstreamer_lz4.c
+++ b/src/bin/pg_basebackup/bbstreamer_lz4.c
@@ -97,7 +97,7 @@ bbstreamer_lz4_compressor_new(bbstreamer *next, pg_compress_specification *compr
return &streamer->base;
#else
- pg_fatal("this build does not support lz4 compression");
+ pg_fatal("this build does not support compression with %s", "LZ4");
return NULL;/* keep compiler quiet */
#endif
}
@@ -295,7 +295,7 @@ bbstreamer_lz4_decompressor_new(bbstreamer *next)
return &streamer->base;
#else
- pg_fatal("this build does not support lz4 compression");
+ pg_fatal("this build does not support compression with %s", "LZ4");
return NULL;/* keep compiler quiet */
#endif
}
diff --git a/src/bin/pg_basebackup/bbstreamer_zstd.c b/src/bin/pg_basebackup/bbstreamer_zstd.c
index 1207dd771a..72cd052c7d 100644
--- a/src/bin/pg_basebackup/bbstreamer_zstd.c
+++ b/src/bin/pg_basebackup/bbstreamer_zstd.c
@@ -113,7 +113,7 @@ bbstreamer_zstd_compressor_new(bbstreamer *next, pg_compress_specification *comp
return &streamer->base;
#else
- pg_fatal("this build does not support zstd compression");
+ pg_fatal("this build does not support compression with %s", "ZSTD");
return NULL;/* keep compiler quiet */
#endif
}
@@ -268,7 +268,7 @@ bbstreamer_zstd_decompressor_new(bbstreamer *next)
return &streamer->base;
#else
- pg_fatal("this build does not support zstd compression");
+ pg_fatal("this build does not support compression with %s", "ZSTD");
return NULL;/* keep compiler quiet */
#endif
}
diff --git a/src/bin/pg_dump/compress_io.c b/src/bin/pg_dump/compress_io.c
index a7df600cc0..26967eb618 100644
--- a/src/bin/pg_dump/compress_io.c
+++ b/src/bin/pg_dump/compress_io.c
@@ -101,7 +101,7 @@ AllocateCompressor(const pg_compress_specification compression_spec,
#ifndef HAVE_LIBZ
if (compression_spec.algorithm == PG_COMPRESSION_GZIP)
- pg_fatal("not built with zlib support");
+ pg_fatal("this build does not support compression with %s", "gzip");
#endif
cs = (CompressorState *) pg_malloc0(sizeof(CompressorState));
@@ -135,7 +135,7 @@ ReadDataFromArchive(ArchiveHandle *AH,
#ifdef HAVE_LIBZ
ReadDataFromArchiveZlib(AH, readF);
#else
- pg_fatal("not built with zlib support");
+ pg_fatal("this build does not support compression with %s", "gzip");
#endif
}
}
@@ -153,7 +153,7 @@ WriteDataToArchive(ArchiveHandle *AH, CompressorState *cs,
#ifdef HAVE_LIBZ
WriteDataToArchiveZlib(AH, cs, data, dLen);
#else
- pg_fatal("not built with zlib support");
+ pg_fatal("this build does not support compression with %s", "gzip");
#endif
break;
case PG_COMPRESSION_NONE:
@@ -482,7 +482,7 @@ cfopen_write(const char *path, const char *mode,
fp = cfopen(fname, mode, compression_spec);
free_keep_errno(fname);
#else
- pg_fatal("not built with zlib support");
+ pg_fatal("this build does not support compression with %s", "gzip");
fp = NULL;/* keep compiler quiet */
#endif
}
@@ -526,7 +526,7 @@ cfopen(const char *path, const char *mode,
fp = NULL;
}
#else
- pg_fatal("not built with zlib support");
+ pg_fatal("this build does not support compression with %s", "gzip");
#endif
}
else
signature.asc
Description: PGP signature
Re: GROUP BY ALL
On Sunday, December 18, 2022, Tom Lane wrote: > Andrey Borodin writes: > > I saw a thread in a social network[0] about GROUP BY ALL. The idea seems > useful. > > Isn't that just a nonstandard spelling of SELECT DISTINCT? > > What would happen if there are aggregate functions in the tlist? > I'm not especially on board with "ALL" meaning "ALL (oh, but not > aggregates)". > > IIUC some systems treat any non-aggregated column as an implicit group by column. This proposal is an explicit way to enable that implicit behavior in PostgreSQL. It is, as you note, an odd meaning for the word ALL. We tend to not accept non-standard usability syntax extensions even if others systems implement them. I don’t see this one ending up being an exception… David J.
Re: GROUP BY ALL
On Sun, Dec 18, 2022 at 8:30 PM Tom Lane wrote: > > I'm not especially on board with "ALL" meaning "ALL (oh, but not > aggregates)". Yes, that's the weak part of the proposal. I even thought about renaming it to "GROUP BY SOMEHOW" or even "GROUP BY SURPRISE ME". I mean I see some cases when it's useful and much less cases when it's dangerously ambiguous. E.g. grouping by result of a subquery looks way too complex and unpredictable. But with simple Vars... what could go wrong? Best regards, Andrey Borodin.
Re: GROUP BY ALL
Andrey Borodin writes: > I saw a thread in a social network[0] about GROUP BY ALL. The idea seems > useful. Isn't that just a nonstandard spelling of SELECT DISTINCT? What would happen if there are aggregate functions in the tlist? I'm not especially on board with "ALL" meaning "ALL (oh, but not aggregates)". regards, tom lane
Re: (non) translatable string splicing
On Fri, Dec 16, 2022 at 07:24:52AM -0600, Justin Pryzby wrote:
> Due to incomplete translation, that allows some pretty fancy output,
> like:
> | You must identify the directory where the residen los binarios del clúster
> antiguo.
>
> That commit also does this a couple times:
>
> +_(" which is an index on \"%s.%s\""),
Ugh. Perhaps we could just simplify the wordings as of "index on
blah", "index on OID %u", "TOAST table for blah" and "TOAST table for
OID %u" with newlines after each item?
--
Michael
signature.asc
Description: PGP signature
GROUP BY ALL
Hi hackers! I saw a thread in a social network[0] about GROUP BY ALL. The idea seems useful. I always was writing something like select datname, usename, count(*) from pg_stat_activity group by 1,2; and then rewriting to select datname, usename, query, count(*) from pg_stat_activity group by 1,2; and then "aaa, add a number at the end". With the proposed feature I can write just select datname, usename, count(*) from pg_stat_activity group by all; PFA very dummy implementation just for a discussion. I think we can add all non-aggregating targets. What do you think? Best regards, Andrey Borodin. [0] https://www.linkedin.com/posts/mosha_duckdb-firebolt-snowflake-activity-7009615821006131200-VQ0o/ v1-0001-Implement-GROUP-BY-ALL.patch Description: Binary data
Re: Add LZ4 compression in pg_dump
On Sat, Dec 17, 2022 at 05:26:15PM -0600, Justin Pryzby wrote: > 001: still refers to "gzip", which is correct for -Fp and -Fd but not > for -Fc, for which it's more correct to say "zlib". Or should we begin by changing all these existing "not built with zlib support" error strings to the more generic "this build does not support compression with %s" to reduce the number of messages to translate? That would bring consistency with the other tools dealing with compression. > That affects the > name of the function, structures, comments, etc. I'm not sure if it's > an issue to re-use the basebackup compression routines here. Maybe we > should accept "-Zn" for zlib output (-Fc), but reject "gzip:9", which > I'm sure some will find confusing, as it does not output. Maybe 001 > should be split into a patch to re-use the existing "cfp" interface > (which is a clear win), and 002 to re-use the basebackup interfaces for > user input and constants, etc. > > 001 still doesn't compile on freebsd, and 002 doesn't compile on > windows. Have you checked test results from cirrusci on your private > github account ? FYI, I have re-added an entry to the CF app to get some automated coverage: https://commitfest.postgresql.org/41/3571/ On MinGW, a complain about the open() callback, which I guess ought to be avoided with a rename: [00:16:37.254] compress_gzip.c:356:38: error: macro "open" passed 4 arguments, but takes just 3 [00:16:37.254] 356 | ret = CFH->open(fname, -1, mode, CFH); [00:16:37.254] | ^ [00:16:37.254] In file included from ../../../src/include/c.h:1309, [00:16:37.254] from ../../../src/include/postgres_fe.h:25, [00:16:37.254] from compress_gzip.c:15: On MSVC, some declaration conflicts, for a similar issue: [00:12:31.966] ../src/bin/pg_dump/compress_io.c(193): error C2371: '_read': redefinition; different basic types [00:12:31.966] C:\Program Files (x86)\Windows Kits\10\include\10.0.20348.0\ucrt\corecrt_io.h(252): note: see declaration of '_read' [00:12:31.966] ../src/bin/pg_dump/compress_io.c(210): error C2371: '_write': redefinition; different basic types [00:12:31.966] C:\Program Files (x86)\Windows Kits\10\include\10.0.20348.0\ucrt\corecrt_io.h(294): note: see declaration of '_write' > 002 breaks "pg_dump -Fc -Z2" because (I think) AllocateCompressor() > doesn't store the passed-in compression_spec. Hmm. This looks like a gap in the existing tests that we'd better fix first. This CI is green on Linux. > 003 still uses and not "lz4.h". This should be , not "lz4.h". -- Michael signature.asc Description: PGP signature
Re: [PATCH] random_normal function
On Sat, Dec 17, 2022 at 05:49:15PM +0100, Fabien COELHO wrote: > Overall, I think that there should be a clearer discussion and plan about > which random functionS postgres should provide to complement the standard > instead of going there… randomly:-) So, what does the specification tells about seeds, normal and random functions? A bunch of DBMSs implement RAND, sometimes RANDOM, SEED or even NORMAL using from time to time specific SQL keywords to do the work. Note that SQLValueFunction made the addition of more returning data types a bit more complicated (not much, still) than the new COERCE_SQL_SYNTAX by going through a mapping function, so the keyword/function mapping is straight-forward. -- Michael signature.asc Description: PGP signature
Re: [BUG] pg_upgrade test fails from older versions.
On Sun, Dec 18, 2022 at 08:56:48PM -0500, Tom Lane wrote: > "Anton A. Melnikov" writes: >> 2) In 60684dd83 and b5d63824 there are two changes in the set of specific >> privileges. >> The thing is that in the privileges.sql test there is REVOKE DELETE command >> which becomes pair of REVOKE ALL and GRANT all specific privileges except >> DELETE >> in the result dump. Therefore, any change in the set of specific privileges >> will lead to >> a non-zero dumps diff. >> To avoid this, i propose to replace any specific GRANT and REVOKE in the >> result dumps with ALL. >> This also made in the patch attached. > > Isn't that likely to mask actual bugs? + # Replace specific privilegies with ALL + $dump_contents =~ s/^(GRANT\s|REVOKE\s)(\S*)\s/$1ALL /mgx; Yes, this would silence some diffs in the dumps taken from the old and the new clusters. It seems to me that it is one of the things where the original dumps have better be tweaked, as this does not cause a hard failure when running pg_upgrade. While thinking about that, an extra idea popped in my mind as it may be interesting to be able to filter out some of the diffs in some contexts. So what about adding in 002_pg_upgrade.pl a small-ish hook in the shape of a new environment variable pointing to a file adds some custom filtering rules? -- Michael signature.asc Description: PGP signature
Re: [BUG] pg_upgrade test fails from older versions.
On Mon, Dec 19, 2022 at 03:50:19AM +0300, Anton A. Melnikov wrote: > +-- The internal format of "aclitem" changed in PostgreSQL version 16 > +-- so replace it with text type > +\if :oldpgversion_le15 > +DO $$ > +DECLARE > +change_aclitem_type TEXT; > +BEGIN > +FOR change_aclitem_type IN > +SELECT 'ALTER TABLE ' || table_schema || '.' || > +table_name || ' ALTER COLUMN ' || > + column_name || ' SET DATA TYPE text;' > +AS change_aclitem_type > +FROM information_schema.columns > +WHERE data_type = 'aclitem' and table_schema != 'pg_catalog' > +LOOP > +EXECUTE change_aclitem_type; > +END LOOP; > +END; > +$$; > +\endif This is forgetting about materialized views, which is something that pg_upgrade would also complain about when checking for relations with aclitems. As far as I can see, the only place in the main regression test suite where we have an aclitem attribute is tab_core_types for HEAD and the stable branches, so it would be enough to do this change. Anyway, wouldn't it be better to use the same conditions as the WITH OIDS queries a few lines above, at least for consistency? Note that check_for_data_types_usage() checks for tables, matviews and indexes. -- Michael signature.asc Description: PGP signature
Re: [BUG] pg_upgrade test fails from older versions.
"Anton A. Melnikov" writes: > 2) In 60684dd83 and b5d63824 there are two changes in the set of specific > privileges. > The thing is that in the privileges.sql test there is REVOKE DELETE command > which becomes pair of REVOKE ALL and GRANT all specific privileges except > DELETE > in the result dump. Therefore, any change in the set of specific privileges > will lead to > a non-zero dumps diff. > To avoid this, i propose to replace any specific GRANT and REVOKE in the > result dumps with ALL. > This also made in the patch attached. Isn't that likely to mask actual bugs? regards, tom lane
Re: [PATCH] Backport perl tests for pg_upgrade from 322becb60
On Mon, Dec 19, 2022 at 04:16:53AM +0300, Anton A. Melnikov wrote: > I have withdrawn the patch with the backport, but then the question is > whether we > will make fixes in older test.sh tests seems to be remains open. > Will we fix it? Justin is not sure if anyone needs this: > https://www.postgresql.org/message-id/67b6b447-e9cb-ebde-4a6b-127aea7ca268%40inbox.ru This introduces an extra maintenance cost over the existing things in the stable branches. > Also found that the test from older versions fails in the current master. > Proposed a fix in a new thread: > https://www.postgresql.org/message-id/49f389ba-95ce-8a9b-09ae-f60650c0e7c7%40inbox.ru Thanks. Yes, this is the change of aclitem from 32b to 64b, which is something that needs some tweaks. So let's fix this one. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Backport perl tests for pg_upgrade from 322becb60
Hello! On 09.12.2022 08:19, Michael Paquier wrote: On Mon, Aug 01, 2022 at 01:02:21AM +0300, Anton A. Melnikov wrote: As far as i understand from this thread: https://www.postgresql.org/message-id/flat/Yox1ME99GhAemMq1%40paquier.xyz, the aim of the perl version for the pg_upgrade tests is to achieve equality of dumps for most cross-versions cases. If so this is the significant improvement as previously in test.sh resulted dumps retained unequal and the user was asked to eyeball them manually during cross upgrades between different major versions. So, the backport of the perl tests also seems preferable to me. I don't really agree with that. These TAP tests are really new development, and it took a few tries to get them completely right (well, as much right as it holds for HEAD). If we were to backport any of this, there is a risk of introducing a bug in what we do with any of that, potentially hiding a issue critical related to pg_upgrade. That's not worth taking a risk for. Saying that, I agree that more needs to be done, but I would limit that only to HEAD and let it mature more into the tree in an incremental fashion. -- I have withdrawn the patch with the backport, but then the question is whether we will make fixes in older test.sh tests seems to be remains open. Will we fix it? Justin is not sure if anyone needs this: https://www.postgresql.org/message-id/67b6b447-e9cb-ebde-4a6b-127aea7ca268%40inbox.ru Also found that the test from older versions fails in the current master. Proposed a fix in a new thread: https://www.postgresql.org/message-id/49f389ba-95ce-8a9b-09ae-f60650c0e7c7%40inbox.ru Would be glad to any remarks. With the best wishes, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
[BUG] pg_upgrade test fails from older versions.
Hello!
Found that pg_upgrade test has broken for upgrades from older versions.
This happened for two reasons.
1) In 7b378237a the format of "aclitem" changed so upgrade from <=15
fails with error:
"Your installation contains the "aclitem" data type in user tables.
The internal format of "aclitem" changed in PostgreSQL version 16
so this cluster cannot currently be upgraded... "
Tried to fix it by changing the column type in the upgrade_adapt.sql.
Please see the patch attached.
2) In 60684dd83 and b5d63824 there are two changes in the set of specific
privileges.
The thing is that in the privileges.sql test there is REVOKE DELETE command
which becomes pair of REVOKE ALL and GRANT all specific privileges except DELETE
in the result dump. Therefore, any change in the set of specific privileges
will lead to
a non-zero dumps diff.
To avoid this, i propose to replace any specific GRANT and REVOKE in the result
dumps with ALL.
This also made in the patch attached.
Would be glad to any remarks.
With best regards,
--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companycommit e2c917694acc7ae20d6a9654de87a9fdb5863103
Author: Anton A. Melnikov
Date: Sun Dec 18 16:23:24 2022 +0300
Replace aclitem with text type in pg_upgrade test due to 7b378237
Replace specific privilegies in dumps with ALL due to b5d63824
and 60684dd8.
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 4cc1469306..d23c4b2253 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -44,6 +44,8 @@ sub filter_dump
$dump_contents =~ s/^\-\-.*//mgx;
# Remove empty lines.
$dump_contents =~ s/^\n//mgx;
+ # Replace specific privilegies with ALL
+ $dump_contents =~ s/^(GRANT\s|REVOKE\s)(\S*)\s/$1ALL /mgx;
my $dump_file_filtered = "${dump_file}_filtered";
open(my $dh, '>', $dump_file_filtered)
diff --git a/src/bin/pg_upgrade/upgrade_adapt.sql b/src/bin/pg_upgrade/upgrade_adapt.sql
index 27c4c7fd01..fac77ec968 100644
--- a/src/bin/pg_upgrade/upgrade_adapt.sql
+++ b/src/bin/pg_upgrade/upgrade_adapt.sql
@@ -19,7 +19,8 @@ SELECT
ver <= 906 AS oldpgversion_le96,
ver <= 1000 AS oldpgversion_le10,
ver <= 1100 AS oldpgversion_le11,
- ver <= 1300 AS oldpgversion_le13
+ ver <= 1300 AS oldpgversion_le13,
+ ver <= 1500 AS oldpgversion_le15
FROM (SELECT current_setting('server_version_num')::int / 100 AS ver) AS v;
\gset
@@ -89,3 +90,24 @@ DROP OPERATOR public.#%# (pg_catalog.int8, NONE);
DROP OPERATOR public.!=- (pg_catalog.int8, NONE);
DROP OPERATOR public.#@%# (pg_catalog.int8, NONE);
\endif
+
+-- The internal format of "aclitem" changed in PostgreSQL version 16
+-- so replace it with text type
+\if :oldpgversion_le15
+DO $$
+DECLARE
+change_aclitem_type TEXT;
+BEGIN
+FOR change_aclitem_type IN
+SELECT 'ALTER TABLE ' || table_schema || '.' ||
+table_name || ' ALTER COLUMN ' ||
+ column_name || ' SET DATA TYPE text;'
+AS change_aclitem_type
+FROM information_schema.columns
+WHERE data_type = 'aclitem' and table_schema != 'pg_catalog'
+LOOP
+EXECUTE change_aclitem_type;
+END LOOP;
+END;
+$$;
+\endif
RE: pg_upgrade: Make testing different transfer modes easier
Hi,
With the addition of --copy option, pg_upgrade now has three possible transfer
mode options. Currently, an error does not occur even if multiple transfer
modes are specified. For example, we can also run "pg_upgrade --link --copy
--clone" command. As discussed in Horiguchi-san's previous email, options like
"--mode=link|copy|clone" can prevent this problem.
The attached patch uses the current implementation and performs a minimum check
to prevent multiple transfer modes from being specified.
Regards,
Noriyoshi Shinoda
-Original Message-
From: Peter Eisentraut
Sent: Saturday, December 17, 2022 2:44 AM
To: Daniel Gustafsson
Cc: PostgreSQL Hackers
Subject: Re: pg_upgrade: Make testing different transfer modes easier
On 14.12.22 10:40, Daniel Gustafsson wrote:
>> On 14 Dec 2022, at 08:04, Peter Eisentraut
>> wrote:
>>
>> On 07.12.22 17:33, Peter Eisentraut wrote:
>>> I think if we want to make this configurable on the fly, and environment
>>> variable would be much easier, like
>>> my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy';
>>
>> Here is an updated patch set that incorporates this idea.
>
> I would prefer a small note about it in src/bin/pg_upgrade/TESTING to
> document it outside of the code, but otherwise LGTM.
>
> + $mode,
> '--check'
> ],
>
> ...
>
> - '-p', $oldnode->port, '-P', $newnode->port
> + '-p', $oldnode->port, '-P', $newnode->port,
> + $mode,
> ],
>
> Minor nitpick, but while in there should we take the opportunity to
> add a trailing comma on the other two array declarations which now ends with
> --check?
> It's good Perl practice and will make the code consistent.
committed with these changes
pg_upgrade_check_mode_v1.diff
Description: pg_upgrade_check_mode_v1.diff
Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
On Sun, Dec 18, 2022 at 04:25:15PM -0800, Ted Yu wrote:
> + * Note: Because this function assumes that the realtion whose OID is
> passed as
>
> Typo: realtion -> relation
Thanks, fixed.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 224614b6ebf2c8d919d8d8500c8f9d6bdaf9a0b6 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Sun, 18 Dec 2022 14:46:21 -0800
Subject: [PATCH v3 1/1] fix maintain privs
---
src/backend/catalog/partition.c | 22 ++
src/backend/catalog/toasting.c| 32 +++
src/backend/commands/cluster.c| 35 +++-
src/backend/commands/indexcmds.c | 10 ++---
src/backend/commands/lockcmds.c | 5 +++
src/backend/commands/tablecmds.c | 41 ++-
src/backend/commands/vacuum.c | 7 ++--
src/include/catalog/partition.h | 1 +
src/include/catalog/pg_class.h| 1 +
src/include/catalog/toasting.h| 1 +
src/include/commands/tablecmds.h | 1 +
.../expected/cluster-conflict-partition.out | 6 ++-
src/test/regress/expected/cluster.out | 4 +-
src/test/regress/expected/vacuum.out | 18
14 files changed, 143 insertions(+), 41 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 79ccddce55..8f462af06b 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -119,6 +119,28 @@ get_partition_parent_worker(Relation inhRel, Oid relid, bool *detach_pending)
return result;
}
+/*
+ * get_partition_root
+ * Obtain the partitioned table of a given relation
+ *
+ * Note: Because this function assumes that the relation whose OID is passed as
+ * an argument and each ancestor will have precisely one parent, it should only
+ * be called when it is known that the relation is a partition.
+ */
+Oid
+get_partition_root(Oid relid)
+{
+ List *ancestors = get_partition_ancestors(relid);
+ Oid root = InvalidOid;
+
+ if (list_length(ancestors) > 0)
+ root = llast_oid(ancestors);
+
+ list_free(ancestors);
+
+ return root;
+}
+
/*
* get_partition_ancestors
* Obtain ancestors of given relation
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 9bc10729b0..f9d264e1e6 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -14,6 +14,7 @@
*/
#include "postgres.h"
+#include "access/genam.h"
#include "access/heapam.h"
#include "access/toast_compression.h"
#include "access/xact.h"
@@ -32,6 +33,7 @@
#include "nodes/makefuncs.h"
#include "storage/lock.h"
#include "utils/builtins.h"
+#include "utils/fmgroids.h"
#include "utils/rel.h"
#include "utils/syscache.h"
@@ -413,3 +415,33 @@ needs_toast_table(Relation rel)
/* Otherwise, let the AM decide. */
return table_relation_needs_toast_table(rel);
}
+
+/*
+ * Get the main relation for the given TOAST table.
+ */
+Oid
+get_toast_parent(Oid relid)
+{
+ Relation rel;
+ SysScanDesc scan;
+ ScanKeyData key[1];
+ Oid result = InvalidOid;
+ HeapTuple tuple;
+
+ rel = table_open(RelationRelationId, AccessShareLock);
+
+ ScanKeyInit(&key[0],
+Anum_pg_class_reltoastrelid,
+BTEqualStrategyNumber, F_OIDEQ,
+ObjectIdGetDatum(relid));
+
+ scan = systable_beginscan(rel, ClassToastRelidIndexId, true, NULL, 1, key);
+ tuple = systable_getnext(scan);
+ if (HeapTupleIsValid(tuple))
+ result = ((Form_pg_class) GETSTRUCT(tuple))->oid;
+ systable_endscan(scan);
+
+ table_close(rel, AccessShareLock);
+
+ return result;
+}
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 8966b75bd1..78b2cd62df 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -80,6 +80,7 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
static List *get_tables_to_cluster(MemoryContext cluster_context);
static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
Oid indexOid);
+static bool cluster_is_permitted_for_relation(Oid relid, Oid userid);
/*---
@@ -366,8 +367,7 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
if (recheck)
{
/* Check that the user still has privileges for the relation */
- if (!object_ownercheck(RelationRelationId, tableOid, save_userid) &&
- pg_class_aclcheck(tableOid, save_userid, ACL_MAINTAIN) != ACLCHECK_OK)
+ if (!cluster_is_permitted_for_relation(tableOid, save_userid))
{
relation_close(OldHeap, AccessExclusiveLock);
goto out;
@@ -1646,8 +1646,7 @@ get_tables_to_cluster(MemoryContext cluster_context)
index = (Form_pg_index) GETSTRUCT(indexTuple);
- if (!object_ownercheck(RelationRelationId, index->indrelid, GetUserId()) &&
- pg_class_aclcheck(index->indrelid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
+ if (!cluster_is_permi
Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
On Sun, Dec 18, 2022 at 3:30 PM Nathan Bossart
wrote:
> Here is a new version of the patch. Besides some cleanup, I added an index
> on reltoastrelid for the toast-to-main-relation lookup. Before I bother
> adjusting the tests and documentation, I'm curious to hear thoughts on
> whether this seems like a viable approach.
>
> On Sat, Dec 17, 2022 at 04:39:29AM -0800, Ted Yu wrote:
> > +cluster_is_permitted_for_relation(Oid relid, Oid userid)
> > +{
> > + return pg_class_aclcheck(relid, userid, ACL_MAINTAIN) ==
> > ACLCHECK_OK ||
> > + has_parent_privs(relid, userid, ACL_MAINTAIN);
> >
> > Since the func only contains one statement, it seems this can be defined
> as
> > a macro instead.
>
> In the new version, there is a bit more to this function, so I didn't
> convert it to a macro.
>
> > + List *ancestors = get_partition_ancestors(relid);
> > + Oid root = InvalidOid;
> >
> > nit: it would be better if the variable `root` can be aligned with
> variable
> > `ancestors`.
>
> Hm. It looked alright on my machine. In any case, I'll be sure to run
> pgindent at some point. This patch is still in early stages.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
Hi,
+ * Note: Because this function assumes that the realtion whose OID is
passed as
Typo: realtion -> relation
Cheers
Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
On Sat, Dec 17, 2022 at 04:39:29AM -0800, Ted Yu wrote: > + List *ancestors = get_partition_ancestors(relid); > + Oid root = InvalidOid; > > nit: it would be better if the variable `root` can be aligned with variable > `ancestors`. It is aligned, but only after configuring one's editor/pager/mail client to display tabs in the manner assumed by postgres' coding style.
Re: wake up logical workers after ALTER SUBSCRIPTION
On Thu, Dec 15, 2022 at 02:47:21PM -0800, Nathan Bossart wrote: > I tried setting wal_retrieve_retry_interval to 1ms for all TAP tests > (similar to what was done in 2710ccd), and I noticed that the recovery > tests consistently took much longer. Upon further inspection, it looks > like the same (or a very similar) race condition described in e5d494d's > commit message [0]. With some added debug logs, I see that all of the > callers of MaybeStartWalReceiver() complete before SIGCHLD is processed, so > ServerLoop() waits for a minute before starting the WAL receiver. > > A simple fix is to have DetermineSleepTime() take the WalReceiverRequested > flag into consideration. The attached 0002 patch shortens the sleep time > to 100ms if it looks like we are waiting on a SIGCHLD. I'm not certain > this is the best approach, but it seems to fix the tests. This seems to have somehow broken the archiving tests on Windows, so obviously I owe some better analysis here. I didn't see anything obvious in the logs, but I will continue to dig. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Standard REGEX functions
Vik Fearing writes: > I don't suppose project policy would allow us to use an external > library. I assume there is one out there that implements XQuery regular > expressions. Probably, but is it in C and does it have a compatible license? The bigger picture here is that our track record with use of external libraries is just terrible: we've outlived the original maintainers' interest multiple times. (That's how we got to the current situation with the regex code, for example.) So I'd want to see a pretty darn vibrant-looking upstream community for any new dependency we adopt. regards, tom lane
Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
Here is a new version of the patch. Besides some cleanup, I added an index
on reltoastrelid for the toast-to-main-relation lookup. Before I bother
adjusting the tests and documentation, I'm curious to hear thoughts on
whether this seems like a viable approach.
On Sat, Dec 17, 2022 at 04:39:29AM -0800, Ted Yu wrote:
> +cluster_is_permitted_for_relation(Oid relid, Oid userid)
> +{
> + return pg_class_aclcheck(relid, userid, ACL_MAINTAIN) ==
> ACLCHECK_OK ||
> + has_parent_privs(relid, userid, ACL_MAINTAIN);
>
> Since the func only contains one statement, it seems this can be defined as
> a macro instead.
In the new version, there is a bit more to this function, so I didn't
convert it to a macro.
> + List *ancestors = get_partition_ancestors(relid);
> + Oid root = InvalidOid;
>
> nit: it would be better if the variable `root` can be aligned with variable
> `ancestors`.
Hm. It looked alright on my machine. In any case, I'll be sure to run
pgindent at some point. This patch is still in early stages.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From f0f65618d1772151b3c4aff6f8da77bd1f212278 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Sun, 18 Dec 2022 14:46:21 -0800
Subject: [PATCH v2 1/1] fix maintain privs
---
src/backend/catalog/partition.c | 22 ++
src/backend/catalog/toasting.c| 32 +++
src/backend/commands/cluster.c| 35 +++-
src/backend/commands/indexcmds.c | 10 ++---
src/backend/commands/lockcmds.c | 5 +++
src/backend/commands/tablecmds.c | 41 ++-
src/backend/commands/vacuum.c | 7 ++--
src/include/catalog/partition.h | 1 +
src/include/catalog/pg_class.h| 1 +
src/include/catalog/toasting.h| 1 +
src/include/commands/tablecmds.h | 1 +
.../expected/cluster-conflict-partition.out | 6 ++-
src/test/regress/expected/cluster.out | 4 +-
src/test/regress/expected/vacuum.out | 18
14 files changed, 143 insertions(+), 41 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 79ccddce55..9a4caa769a 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -119,6 +119,28 @@ get_partition_parent_worker(Relation inhRel, Oid relid, bool *detach_pending)
return result;
}
+/*
+ * get_partition_root
+ * Obtain the partitioned table of a given relation
+ *
+ * Note: Because this function assumes that the realtion whose OID is passed as
+ * an argument and each ancestor will have precisely one parent, it should only
+ * be called when it is known that the relation is a partition.
+ */
+Oid
+get_partition_root(Oid relid)
+{
+ List *ancestors = get_partition_ancestors(relid);
+ Oid root = InvalidOid;
+
+ if (list_length(ancestors) > 0)
+ root = llast_oid(ancestors);
+
+ list_free(ancestors);
+
+ return root;
+}
+
/*
* get_partition_ancestors
* Obtain ancestors of given relation
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 9bc10729b0..f9d264e1e6 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -14,6 +14,7 @@
*/
#include "postgres.h"
+#include "access/genam.h"
#include "access/heapam.h"
#include "access/toast_compression.h"
#include "access/xact.h"
@@ -32,6 +33,7 @@
#include "nodes/makefuncs.h"
#include "storage/lock.h"
#include "utils/builtins.h"
+#include "utils/fmgroids.h"
#include "utils/rel.h"
#include "utils/syscache.h"
@@ -413,3 +415,33 @@ needs_toast_table(Relation rel)
/* Otherwise, let the AM decide. */
return table_relation_needs_toast_table(rel);
}
+
+/*
+ * Get the main relation for the given TOAST table.
+ */
+Oid
+get_toast_parent(Oid relid)
+{
+ Relation rel;
+ SysScanDesc scan;
+ ScanKeyData key[1];
+ Oid result = InvalidOid;
+ HeapTuple tuple;
+
+ rel = table_open(RelationRelationId, AccessShareLock);
+
+ ScanKeyInit(&key[0],
+Anum_pg_class_reltoastrelid,
+BTEqualStrategyNumber, F_OIDEQ,
+ObjectIdGetDatum(relid));
+
+ scan = systable_beginscan(rel, ClassToastRelidIndexId, true, NULL, 1, key);
+ tuple = systable_getnext(scan);
+ if (HeapTupleIsValid(tuple))
+ result = ((Form_pg_class) GETSTRUCT(tuple))->oid;
+ systable_endscan(scan);
+
+ table_close(rel, AccessShareLock);
+
+ return result;
+}
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 8966b75bd1..78b2cd62df 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -80,6 +80,7 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
static List *get_tables_to_cluster(MemoryContext cluster_context);
static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
Oid indexOid);
+static bool cluster_is_per
Re: Standard REGEX functions
On 12/18/22 15:24, Tom Lane wrote: Vik Fearing writes: Are there any objections to me writing a patch to add SQL Standard regular expression functions even though they call for XQuery and we would use our own language? Yes. If we provide spec-defined syntax it should have spec-defined behavior. I really don't see the value of providing different syntactic sugar for functionality we already have, unless the point of it is to be spec-compliant, and what you suggest is exactly not that. I was expecting this answer and I can't say I disagree with it. I recall having looked at the points of inconsistency (see 9.7.3.8) Oh sweet! I was not aware of that section. and thought that we could probably create an option flag for our regex engine that would address them, or at least get pretty close. It'd take some work though, especially for somebody who never looked at that code before. Yeah. If I had the chops to do this, I would have tackled row pattern recognition long ago. I don't suppose project policy would allow us to use an external library. I assume there is one out there that implements XQuery regular expressions. -- Vik Fearing
Re: Transaction timeout
On Wed, Dec 7, 2022 at 1:30 PM Andrey Borodin wrote:
> I hope to address other feedback on the weekend.
Andres, here's my progress on working with your review notes.
> > @@ -3277,6 +3282,7 @@ ProcessInterrupts(void)
> >*/
> > lock_timeout_occurred = get_timeout_indicator(LOCK_TIMEOUT,
> > true);
> > stmt_timeout_occurred =
> > get_timeout_indicator(STATEMENT_TIMEOUT, true);
> > + tx_timeout_occurred =
> > get_timeout_indicator(TRANSACTION_TIMEOUT, true);
> >
> > /*
> >* If both were set, we want to report whichever timeout
> > completed
>
> This doesn't update the preceding comment, btw, which now reads oddly:
I've rewritten this part to correctly report all timeouts that did
happen. However there's now a tricky comma-formatting code which was
tested only manually.
> > > > @@ -1360,6 +1363,16 @@ IdleInTransactionSessionTimeoutHandler(void)
> > > > SetLatch(MyLatch);
> > > > }
> > > >
> > > > +static void
> > > > +TransactionTimeoutHandler(void)
> > > > +{
> > > > +#ifdef HAVE_SETSID
> > > > + /* try to signal whole process group */
> > > > + kill(-MyProcPid, SIGINT);
> > > > +#endif
> > > > + kill(MyProcPid, SIGINT);
> > > > +}
> > > > +
> > >
> > > Why does this use signals instead of just setting the latch like
> > > IdleInTransactionSessionTimeoutHandler() etc?
> >
> > I just copied statement_timeout behaviour. As I understand this
> > implementation is prefered if the timeout can catch the backend
> > running at full steam.
>
> Hm. I'm not particularly convinced by that code. Be that as it may, I
> don't think it's a good idea to have one more copy of this code. At
> least the patch should wrap the signalling code in a helper.
Done, now there is a single CancelOnTimeoutHandler() handler.
> > > > diff --git a/src/bin/pg_dump/pg_backup_archiver.c
> > > > b/src/bin/pg_dump/pg_backup_archiver.c
> > > > index 0081873a72..5229fe3555 100644
> > > > --- a/src/bin/pg_dump/pg_backup_archiver.c
> > > > +++ b/src/bin/pg_dump/pg_backup_archiver.c
> > > > @@ -3089,6 +3089,7 @@ _doSetFixedOutputState(ArchiveHandle *AH)
> > > > ahprintf(AH, "SET statement_timeout = 0;\n");
> > > > ahprintf(AH, "SET lock_timeout = 0;\n");
> > > > ahprintf(AH, "SET idle_in_transaction_session_timeout = 0;\n");
> > > > + ahprintf(AH, "SET transaction_timeout = 0;\n");
> > >
> > > Hm - why is that the right thing to do?
> > Because transaction_timeout has effects of statement_timeout.
>
> I guess it's just following precedent - but it seems a bit presumptuous
> to just disable safety settings a DBA might have set up. That makes some
> sense for e.g. idle_in_transaction_session_timeout, because I think
> e.g. parallel backup can lead to a connection being idle for a bit.
I do not know. My reasoning - everywhere we turn off
statement_timeout, we should turn off transaction_timeout too.
But I have no strong opinion here. I left this code as is in the patch
so far. For the same reason I did not change anything in
pg_backup_archiver.c.
> > Either way we can do batch function enable_timeouts() instead
> > enable_timeout_after().
>
> > Does anything of it make sense?
>
> I'm at least as worried about the various calls *after* the execution of
> a statement.
I think this code is just a one bit check
if (get_timeout_active(TRANSACTION_TIMEOUT))
inside of get_timeout_active(). With all 14 timeouts we have, I don't
see a good way to optimize stuff so far.
> > + if (tx_timeout_occurred)
> > + {
> > + LockErrorCleanup();
> > + ereport(ERROR,
> > + (errcode(ERRCODE_TRANSACTION_TIMEOUT),
> > + errmsg("canceling transaction due to
> > transaction timeout")));
> > + }
>
> The number of calls to LockErrorCleanup() here feels wrong - there's
> already 8 calls in ProcessInterrupts(). Besides the code duplication I
> also think it's not a sane idea to rely on having LockErrorCleanup()
> before all the relevant ereport(ERROR)s.
I've refactored that code down to 7 calls of LockErrorCleanup() :)
Logic behind various branches is not clear for me, e.g. why we do not
LockErrorCleanup() when reading commands from a client?
So I did not risk refactoring further.
> I think the test should verify
> that transaction timeout interacts correctly with statement timeout /
> idle in tx timeout.
I've added tests that check statement_timeout vs transaction_timeout.
However I could not produce stable tests with
idle_in_transaction_timeout vs transaction_timeout so far. But I'll
look into this more.
Actually, stabilizing statement_timeout vs transaction_timeout was
tricky on Windows too. I had to remove the second call to
pg_sleep(0.0001) because it was triggering 10ьs timeout from time to
time. Also, test timeout was increased to 30ms, because unlike others
in spec it's not supposed to h
Re: Error-safe user functions
On 2022-12-14 We 17:37, Tom Lane wrote:
> Andrew Dunstan writes:
>> Thanks, I have been looking at jsonpath, but I'm not quite sure how to
>> get the escontext argument to the yyerror calls in jsonath_scan.l. Maybe
>> I need to specify a lex-param setting?
> You want a parse-param option in jsonpath_gram.y, I think; adding that
> will persuade Bison to change the signatures of relevant functions.
> Compare the mods I made in contrib/cube in ccff2d20e.
>
>
Yeah, I started there, but it's substantially more complex - unlike cube
the jsonpath scanner calls the error routines as well as the parser.
Anyway, here's a patch.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/src/backend/utils/adt/jsonpath.c b/src/backend/utils/adt/jsonpath.c
index 91af030095..6c7a5b9854 100644
--- a/src/backend/utils/adt/jsonpath.c
+++ b/src/backend/utils/adt/jsonpath.c
@@ -66,16 +66,19 @@
#include "funcapi.h"
#include "lib/stringinfo.h"
#include "libpq/pqformat.h"
+#include "nodes/miscnodes.h"
#include "miscadmin.h"
#include "utils/builtins.h"
#include "utils/json.h"
#include "utils/jsonpath.h"
-static Datum jsonPathFromCstring(char *in, int len);
+static Datum jsonPathFromCstring(char *in, int len, struct Node *escontext);
static char *jsonPathToCstring(StringInfo out, JsonPath *in,
int estimated_len);
-static int flattenJsonPathParseItem(StringInfo buf, JsonPathParseItem *item,
+static bool flattenJsonPathParseItem(StringInfo buf, int *result,
+ struct Node *escontext,
+ JsonPathParseItem *item,
int nestingLevel, bool insideArraySubscript);
static void alignStringInfoInt(StringInfo buf);
static int32 reserveSpaceForItemPointer(StringInfo buf);
@@ -95,7 +98,7 @@ jsonpath_in(PG_FUNCTION_ARGS)
char *in = PG_GETARG_CSTRING(0);
int len = strlen(in);
- return jsonPathFromCstring(in, len);
+ return jsonPathFromCstring(in, len, fcinfo->context);
}
/*
@@ -119,7 +122,7 @@ jsonpath_recv(PG_FUNCTION_ARGS)
else
elog(ERROR, "unsupported jsonpath version number: %d", version);
- return jsonPathFromCstring(str, nbytes);
+ return jsonPathFromCstring(str, nbytes, NULL);
}
/*
@@ -165,24 +168,29 @@ jsonpath_send(PG_FUNCTION_ARGS)
* representation of jsonpath.
*/
static Datum
-jsonPathFromCstring(char *in, int len)
+jsonPathFromCstring(char *in, int len, struct Node *escontext)
{
- JsonPathParseResult *jsonpath = parsejsonpath(in, len);
+ JsonPathParseResult *jsonpath = parsejsonpath(in, len, escontext);
JsonPath *res;
StringInfoData buf;
- initStringInfo(&buf);
- enlargeStringInfo(&buf, 4 * len /* estimation */ );
-
- appendStringInfoSpaces(&buf, JSONPATH_HDRSZ);
+ if (SOFT_ERROR_OCCURRED(escontext))
+ return (Datum) NULL;
if (!jsonpath)
- ereport(ERROR,
+ ereturn(escontext, (Datum) NULL,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("invalid input syntax for type %s: \"%s\"", "jsonpath",
in)));
- flattenJsonPathParseItem(&buf, jsonpath->expr, 0, false);
+ initStringInfo(&buf);
+ enlargeStringInfo(&buf, 4 * len /* estimation */ );
+
+ appendStringInfoSpaces(&buf, JSONPATH_HDRSZ);
+
+ if (!flattenJsonPathParseItem(&buf, NULL, escontext,
+ jsonpath->expr, 0, false))
+ return (Datum) NULL;
res = (JsonPath *) buf.data;
SET_VARSIZE(res, buf.len);
@@ -225,9 +233,10 @@ jsonPathToCstring(StringInfo out, JsonPath *in, int estimated_len)
* Recursive function converting given jsonpath parse item and all its
* children into a binary representation.
*/
-static int
-flattenJsonPathParseItem(StringInfo buf, JsonPathParseItem *item,
- int nestingLevel, bool insideArraySubscript)
+static bool
+flattenJsonPathParseItem(StringInfo buf, int *result, struct Node *escontext,
+ JsonPathParseItem *item, int nestingLevel,
+ bool insideArraySubscript)
{
/* position from beginning of jsonpath data */
int32 pos = buf->len - JSONPATH_HDRSZ;
@@ -295,16 +304,22 @@ flattenJsonPathParseItem(StringInfo buf, JsonPathParseItem *item,
int32 left = reserveSpaceForItemPointer(buf);
int32 right = reserveSpaceForItemPointer(buf);
-chld = !item->value.args.left ? pos :
- flattenJsonPathParseItem(buf, item->value.args.left,
- nestingLevel + argNestingLevel,
- insideArraySubscript);
+if (!item->value.args.left)
+ chld = pos;
+else if (! flattenJsonPathParseItem(buf, &chld, escontext,
+ item->value.args.left,
+ nestingLevel + argNestingLevel,
+ insideArraySubscript))
+ return false;
*(int32 *) (buf->data + left) = chld - pos;
-chld = !item->value.args.right ? pos :
- flattenJsonPathParseItem(buf, item->value.args.right,
- nestingLevel + argNestingLevel,
- insideArraySubscript);
+if (!item->value.args.right)
+ chld = pos;
+else if (! flattenJsonPathParseItem(buf, &chld, escontext,
+ item->value.args.rig
Re: Standard REGEX functions
Vik Fearing writes: > Are there any objections to me writing a patch to add SQL Standard > regular expression functions even though they call for XQuery and we > would use our own language? Yes. If we provide spec-defined syntax it should have spec-defined behavior. I really don't see the value of providing different syntactic sugar for functionality we already have, unless the point of it is to be spec-compliant, and what you suggest is exactly not that. I recall having looked at the points of inconsistency (see 9.7.3.8) and thought that we could probably create an option flag for our regex engine that would address them, or at least get pretty close. It'd take some work though, especially for somebody who never looked at that code before. I'd be willing to blow off the locale discrepancies by continuing to say that you have to use an appropriate locale, and I think the business around varying newline representations is in the way-more- trouble-than-its-worth department. But we should at least match the spec on available escape sequences and flag names. It would be a seriously bad idea, for example, if the default does-dot-match-newline behavior wasn't per spec. regards, tom lane
Standard REGEX functions
The standard uses XQuery regular expressions, which I believe are subtly different from ours. Because of that, I had been hesitant to add some standard functions to our library such as . While looking at commit 6424337073589476303b10f6d7cc74f501b8d9d7 from last year (which will come up soon from somebody else for a different reason), I noticed that we added those functions for Oracle compatibility even though the regexp language was not the same. Are there any objections to me writing a patch to add SQL Standard regular expression functions even though they call for XQuery and we would use our own language? -- Vik Fearing
pg_dump/pg_restore: Fix stdin/stdout handling of custom format on Win32
Well, this is embarassing. Sorry for the inconvenience. Some part
of my company's network infrastruture must have mangled the attachment.
Both mails were sent using a combination of git format-patch
and git send-email. However, as this is my first foray into this
email-based workflow, I won't rule out a failure on my part. Bear
with me and let's try again. diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 7f7a0f1ce7..684db4a17a 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3838,6 +3838,18 @@ bool
checkSeek(FILE *fp)
{
pgoff_t tpos;
+ struct stat st;
+
+ /* Check if this is a terminal descriptor */
+ if (isatty(fileno(fp))) {
+ return false;
+ }
+
+ /* Check if this is an unseekable character special device or pipe */
+ if ((fstat(fileno(fp), &st) == 0) && (S_ISCHR(st.st_mode)
+ || S_ISFIFO(st.st_mode))) {
+ return false;
+ }
/* Check that ftello works on this file */
tpos = ftello(fp);
diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h
index 296905bc6c..892d0bb401 100644
--- a/src/include/port/win32_port.h
+++ b/src/include/port/win32_port.h
@@ -329,6 +329,12 @@ extern int _pglstat64(const char *name, struct stat *buf);
#ifndef S_ISREG
#define S_ISREG(m) (((m) & S_IFMT) == S_IFREG)
#endif
+#ifndef S_ISFIFO
+#define S_ISFIFO(m) (((m) & S_IFMT) == _S_IFIFO)
+#endif
+#ifndef S_ISCHR
+#define S_ISCHR(m) (((m) & S_IFMT) == S_IFCHR)
+#endif
/*
* In order for lstat() to be able to report junction points as symlinks, we
diff --git a/src/port/win32stat.c b/src/port/win32stat.c
index e6553e4030..7ab0983a74 100644
--- a/src/port/win32stat.c
+++ b/src/port/win32stat.c
@@ -256,35 +256,65 @@ _pgstat64(const char *name, struct stat *buf)
int
_pgfstat64(int fileno, struct stat *buf)
{
- HANDLE hFile = (HANDLE) _get_osfhandle(fileno);
- BY_HANDLE_FILE_INFORMATION fiData;
+ HANDLE hFile = (HANDLE) _get_osfhandle(fileno);
+ DWORD fileType = FILE_TYPE_UNKNOWN;
+ DWORD lastError;
+ unsigned short st_mode;
- if (hFile == INVALID_HANDLE_VALUE || buf == NULL)
+ if (hFile == INVALID_HANDLE_VALUE || hFile == (HANDLE)-2 || buf == NULL)
{
errno = EINVAL;
return -1;
}
- /*
- * Check if the fileno is a data stream. If so, unless it has been
- * redirected to a file, getting information through its HANDLE will fail,
- * so emulate its stat information in the most appropriate way and return
- * it instead.
+ fileType = GetFileType(hFile);
+ lastError = GetLastError();
+
+ /*
+ * Invoke GetLastError in order to distinguish between a "valid"
+ * return of FILE_TYPE_UNKNOWN and its return due to a calling error.
+ * In case of success, GetLastError returns NO_ERROR.
*/
- if ((fileno == _fileno(stdin) ||
- fileno == _fileno(stdout) ||
- fileno == _fileno(stderr)) &&
- !GetFileInformationByHandle(hFile, &fiData))
+ if (fileType == FILE_TYPE_UNKNOWN && lastError != NO_ERROR) {
+ _dosmaperr(lastError);
+ return -1;
+ }
+
+ switch (fileType)
{
- memset(buf, 0, sizeof(*buf));
- buf->st_mode = _S_IFCHR;
- buf->st_dev = fileno;
- buf->st_rdev = fileno;
- buf->st_nlink = 1;
- return 0;
+ /* The specified file is a disk file */
+ case FILE_TYPE_DISK:
+ return fileinfo_to_stat(hFile, buf);
+ /*
+ * The specified file is a socket,
+ * a named pipe, or an anonymous pipe.
+ */
+ case FILE_TYPE_PIPE:
+ st_mode = _S_IFIFO;
+ break;
+ /*
+ * The specified file is a character file,
+ * typically an LPT device or a console
+ */
+ case FILE_TYPE_CHAR:
+ /*
+ * Unused flag and unknown file type
+ */
+ case FILE_TYPE_REMOTE:
+ case FILE_TYPE_UNKNOWN:
+ st_mode = _S_IFCHR;
+ break;
+ default:
+ errno = EINVAL;
+ return -1;
}
- return fileinfo_to_stat(hFile, buf);
+ memset(buf, 0, sizeof(*buf));
+ buf->st_mode = st_mode;
+ buf->st_dev = fileno;
+ buf->st_rdev = fileno;
+ buf->st_nlink = 1;
+ return 0;
}
#endif /* WIN32 */
base-commit: e52f8b301ed54aac5162b185b43f5f1e44b6b17e
