Add id's to various elements in protocol.sgml
When working with the Frontend/Backend Protocol implementation in Npgsql
and discussing things with the team, I often struggle with the fact that
you can't set deep links to individual message formats in the somewhat
lengthy html docs pages.
The attached patch adds id's to various elements in protocol.sgml to
make them more accesssible via the public html documentation interface.
I've tried to follow the style that was already there in a few of the
elements.
Do you consider this useful and worth merging?
Is there anything I can improve?
Best Regards,
Brar
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 34a7034282..e22dc5f3b0 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -1810,7 +1810,7 @@ Replication commands are logged in the server log when
The commands accepted in replication mode are:
-
+
IDENTIFY_SYSTEM
IDENTIFY_SYSTEM
@@ -1875,7 +1875,7 @@ The commands accepted in replication mode are:
-
+
SHOW name
SHOW
@@ -1899,7 +1899,7 @@ The commands accepted in replication mode are:
-
+
TIMELINE_HISTORY tli
TIMELINE_HISTORY
@@ -2084,7 +2084,7 @@ The commands accepted in replication mode are:
-
+
CREATE_REPLICATION_SLOT slot_name [ TEMPORARY ] {
PHYSICAL [ RESERVE_WAL ] |
LOGICAL output_plugin [
EXPORT_SNAPSHOT | NOEXPORT_SNAPSHOT |
USE_SNAPSHOT | TWO_PHASE ] }
@@ -2095,7 +2095,7 @@ The commands accepted in replication mode are:
-
+
READ_REPLICATION_SLOT slot_name
READ_REPLICATION_SLOT
@@ -2143,7 +2143,7 @@ The commands accepted in replication mode are:
-
+
START_REPLICATION [ SLOT
slot_name ] [
PHYSICAL ] XXX/XXX [ TIMELINE
tli ]
START_REPLICATION
@@ -2201,7 +2201,7 @@ The commands accepted in replication mode are:
-
+
XLogData (B)
@@ -2270,7 +2270,7 @@ The commands accepted in replication mode are:
-
+
Primary keepalive message (B)
@@ -2334,7 +2334,7 @@ The commands accepted in replication mode are:
-
+
Standby status update (F)
@@ -2415,7 +2415,7 @@ The commands accepted in replication mode are:
-
+
Hot Standby feedback message (F)
@@ -2497,7 +2497,7 @@ The commands accepted in replication mode are:
-
+
START_REPLICATION SLOT
slot_name
LOGICAL XXX/XXX
[ ( option_name [
option_value ] [, ...] ) ]
@@ -2572,7 +2572,7 @@ The commands accepted in replication mode are:
-
+
DROP_REPLICATION_SLOT slot_name WAIT
DROP_REPLICATION_SLOT
@@ -2886,7 +2886,7 @@ The commands accepted in replication mode are:
-
+
BASE_BACKUP [ LABEL
'label' ] [ PROGRESS ] [
FAST ] [ WAL ] [
NOWAIT ] [ MAX_RATE
rate ] [ TABLESPACE_MAP ] [
NOVERIFY_CHECKSUMS ] [ MANIFEST
manifest_option ] [
MANIFEST_CHECKSUMS
checksum_algorithm ]
@@ -3138,7 +3138,7 @@ of any individual CopyData message cannot be
interpretable on their own.)
-
+
AuthenticationOk (B)
@@ -3183,7 +3183,7 @@ AuthenticationOk (B)
-
+
AuthenticationKerberosV5 (B)
@@ -3227,7 +3227,7 @@ AuthenticationKerberosV5 (B)
-
+
AuthenticationCleartextPassword (B)
@@ -3271,7 +3271,7 @@ AuthenticationCleartextPassword (B)
-
+
AuthenticationMD5Password (B)
@@ -3326,7 +3326,7 @@ AuthenticationMD5Password (B)
-
+
AuthenticationSCMCredential (B)
@@ -3371,7 +3371,7 @@ AuthenticationSCMCredential (B)
-
+
AuthenticationGSS (B)
@@ -3416,7 +3416,7 @@ AuthenticationGSS (B)
-
+
AuthenticationGSSContinue (B)
@@ -3471,7 +3471,7 @@ AuthenticationGSSContinue (B)
-
+
AuthenticationSSPI (B)
@@ -3516,7 +3516,7 @@ AuthenticationSSPI (B)
-
+
AuthenticationSASL (B)
@@ -3577,7 +3577,7 @@ following:
-
+
AuthenticationSASLContinue (B)
@@ -3632,7 +3632,7 @@ AuthenticationSASLContinue (B)
-
+
AuthenticationSASLFinal (B)
@@ -3688,7 +3688,7 @@ AuthenticationSASLFinal (B)
-
+
BackendKeyData (B)
@@ -3745,7 +3745,7 @@ BackendKeyData (B)
-
+
Bind (F)
@@ -3898,7 +3898,7 @@ Bind (F)
-
+
BindComplete (B)
@@ -3933,7 +3933,7 @@ BindComplete (B)
-
+
CancelRequest (F)
@@ -3991,7 +3991,7 @@ CancelRequest (F)
-
+
Close (F)
@@ -4048,7 +4048,7 @@ Close (F)
-
+
CloseComplete (B)
@@ -4083,7 +4083,7 @@ CloseComplete (B)
-
+
CommandComplete (B)
@@ -4182,7 +4182,7 @@ CommandComplete (B)
-
+
CopyData (F & B)
@@ -4228,7 +4228,7 @@ CopyData (F & B)
-
+
CopyDone (F & B)
@@ -4263,7 +4263,7 @@ CopyDone (F & B)
-
+
CopyFail (F)
@@ -4308,7 +4308,7 @@ CopyFail (F)
-
+
CopyInResponse (B)
@@ -4384,7 +4384,7 @@ Cop
Re: Add id's to various elements in protocol.sgml
> On 5 Dec 2021, at 16:51, Brar Piening wrote: > The attached patch adds id's to various elements in protocol.sgml to > make them more accesssible via the public html documentation interface. Off the cuff without having checked the compiled results yet, it seems like a good idea. — Daniel Gustafsson
Re: [PATCH] Don't block HOT update by BRIN index
On Tue, Nov 30, 2021 at 08:11:03PM +0100, Tomas Vondra wrote:
> OK,
>
> I've polished the last version of the patch a bit (added a regression test
> with update of attribute in index predicate and docs about the new flag into
> indexam.sgml) and pushed.
brin.sql's new brin_hot test is failing sometimes.
I saw a local failure and then found this.
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=serinus&dt=2021-12-01%2003%3A00%3A07
SELECT pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid);
pg_stat_get_tuples_hot_updated
- 1
+ 0
(1 row)
Evidently because:
| 2021-12-01 04:02:01.096 CET [61a6e587.3106b1:4] LOG: wait_for_hot_stats
delayed 33.217301 seconds
It seems like maybe the UDP packet lost to the stats collector got lost ?
It fails less than 10% of the time here, probably depending on load.
BTW there's a typo in brin.sql: precicates
--
Justin
enable certain TAP tests for MSVC builds
Certain TAP tests rely on settings that the Make files provide for them.
However vcregress.pl doesn't provide those settings. This patch remedies
that, and I propose to apply it shortly (when we have a fix for the SSL
tests that I will write about separately) and backpatch it appropriately.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index f3357b3292..608b0449a3 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -59,6 +59,12 @@ copy("$Config/autoinc/autoinc.dll", "src/test/regress");
copy("$Config/regress/regress.dll", "src/test/regress");
copy("$Config/dummy_seclabel/dummy_seclabel.dll", "src/test/regress");
+# Settings used by TAP tests
+$ENV{with_ssl} = $config->{openssl} ? 'openssl' : 'no';
+$ENV{with_ldap} = $config->{ldap} ? 'yes' : 'no';
+$ENV{with_icu} = $config->{icu} ? 'yes' : 'no';
+$ENV{with_gssapi} = $config->{gss} ? 'yes' : 'no';
+
$ENV{PATH} = "$topdir/$Config/libpq;$ENV{PATH}";
if ($ENV{PERL5LIB})
Re: The "char" type versus non-ASCII characters
Chapman Flack writes: > On 12/04/21 11:34, Tom Lane wrote: >> So I'm visualizing it as a uint8 that we happen to like to store >> ASCII codes in, and that's what prompts the thought of using a >> numeric representation for non-ASCII values. > I'm in substantial agreement, though I also see that it is nearly always > set from a quoted literal, and tested against a quoted literal, and calls > itself "char", so I guess I am thinking for consistency's sake it might > be better not to invent some all-new convention for its text representation, > but adopt something that's already familiar, like bytea escaped format. > So it would always look and act like a one-octet bytea. Hmm. I don't have any great objection to that ... except that I observe that bytea rejects a bare backslash: regression=# select '\'::bytea; ERROR: invalid input syntax for type bytea which would be incompatible with "char"'s existing behavior. But as long as we don't do that, I'd be okay with having high-bit-set char values map to backslash-followed-by-three-octal-digits, which is what bytea escape format would produce. > Maybe have charin > accept either bytea-escaped or bytea-hex form too. That seems like more complexity than is warranted, although I suppose that allowing easy interchange between char and bytea is worth something. One other point in this area is that charin does not currently object to multiple input characters, it just discards the extra: regression=# select 'foo'::"char"; char -- f (1 row) I think that was justified by analogy to regression=# select 'foo'::char(1); bpchar f (1 row) but I think it would be a bad idea to preserve it once we introduce any sort of mapping, because it'd mask mistakes. So I'm envisioning that charin should accept any single-byte string (including non-ASCII, for backwards compatibility), but for multi-byte input throw an error if it doesn't look like whatever numeric-ish mapping we settle on. >> Yup, cstring is definitely presumed to be in the server's encoding. > Without proposing to change it, I observe that by defining both cstring > and unknown in this way (with the latter being expressly the type of > any literal from the client destined for a type we don't know yet), we're > a bit painted into the corner as far as supporting types like NCHAR. Yeah, I'm not sure what to do about that. We convert the query text to server encoding before ever attempting to parse it, and I don't think I want to contemplate trying to postpone that (... especially not if the client encoding is an unsafe one like SJIS, as you probably could not avoid SQL-injection hazards). So an in-line literal in some other encoding is basically impossible, or at least pointless. I'm inclined to think that NCHAR is another one in a rather long list of not-that-well-thought-out SQL features. regards, tom lane
MSVC SSL test failure
I am getting this test failure 001_ssltests.pl on my test MSVC system when SSL tests are enabled: not ok 110 - certificate authorization fails with revoked client cert with server-side CRL directory: matches # Failed test 'certificate authorization fails with revoked client cert with server-side CRL directory: matches' # at t/001_ssltests.pl line 618. # 'psql: error: connection to server at "127.0.0.1", port 59491 failed: server closed the connection unexpectedly # This probably means the server terminated abnormally # before or while processing the request. # server closed the connection unexpectedly # This probably means the server terminated abnormally # before or while processing the request. # server closed the connection unexpectedly # This probably means the server terminated abnormally # before or while processing the request.' # doesn't match '(?^:SSL error: sslv3 alert certificate revoked)' There's nothing terribly suspicious in the server log, so I'm not quite sure what's going on here. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: MSVC SSL test failure
Andrew Dunstan writes: > I am getting this test failure 001_ssltests.pl on my test MSVC system > when SSL tests are enabled: > not ok 110 - certificate authorization fails with revoked client cert > with server-side CRL directory: matches > # Failed test 'certificate authorization fails with revoked client cert > with server-side CRL directory: matches' > # at t/001_ssltests.pl line 618. > # 'psql: error: connection to server at "127.0.0.1", > port 59491 failed: server closed the connection unexpectedly > # This probably means the server terminated abnormally > # before or while processing the request. > # server closed the connection unexpectedly > # This probably means the server terminated abnormally > # before or while processing the request. > # server closed the connection unexpectedly > # This probably means the server terminated abnormally > # before or while processing the request.' > # doesn't match '(?^:SSL error: sslv3 alert certificate revoked)' > There's nothing terribly suspicious in the server log, so I'm not quite > sure what's going on here. Hmm .. does enabling log_connections/log_disconnections produce any useful info? This looks quite a bit like the sort of failure that commit 6051857fc was meant to forestall. I wonder whether reverting that commit changes the results? You might also try inserting a shutdown() call, as we'd decided not to do [1]. regards, tom lane [1] https://www.postgresql.org/message-id/1283317.1638213407%40sss.pgh.pa.us
Re: The "char" type versus non-ASCII characters
On 12/05/21 12:01, Tom Lane wrote: > regression=# select '\'::bytea; > ERROR: invalid input syntax for type bytea > > which would be incompatible with "char"'s existing behavior. But as > long as we don't do that, I'd be okay with having high-bit-set char > values map to backslash-followed-by-three-octal-digits, which is > what bytea escape format would produce. Is that a proposal to change nothing about the current treatment of values < 128, or just to avoid rejecting bare '\'? It seems defensible to relax the error treatment of bare backslash, as it isn't inherently ambiguous; it functions more as an "are you sure you weren't starting to write an escape sequence here?" check. If it's a backslash with nothing after it and you assume the user wrote what they meant, then it's not hard to tell what they meant. If there's a way to factor out and reuse the good parts of byteain, that would mean '\\' would also be accepted to mean a backslash, and the \r \n \t usual escapes would be accepted too, and \ooo and \xhh. >> Maybe have charin >> accept either bytea-escaped or bytea-hex form too. > > That seems like more complexity than is warranted I think it ends up being no more complexity at all, because a single octet in bytea-hex form looks like \xhh, which is exactly what a single \xhh in bytea-escape form looks like. I suppose it's important to consider what comparisons like c = '\' and c = '\\' mean, which should be just fine when the type analysis produces char = char or char = unknown, but could be surprising if it doesn't. Regards, -Chap
ExecTypeSetColNames is fundamentally broken
Changing
Fcc: +inbox
I looked into the failure reported at [1]. Basically what's happening
there is that we're allowing a composite datum of type RECORD to get
stored into a table, whereupon other backends can't make sense of it
since they lack the appropriate typcache entry. The reason the datum
is marked as type RECORD is that ExecTypeSetColNames set things up
that way, after observing that the tuple descriptor obtained from
the current table definition didn't have the column names it thought
it should have.
Now, in the test case as given, ExecTypeSetColNames is in error to
think that, because it fails to account for the possibility that the
tupdesc contains dropped columns that weren't dropped when the relevant
RTE was made. However, if the test case is modified so that we just
rename rather than drop some pre-existing column, then even with a
fix for that problem ExecTypeSetColNames would do the wrong thing.
I made several attempts to work around this problem, but I've now
concluded that changing the type OID in ExecTypeSetColNames is just
fundamentally broken. It can't be okay to decide that a Var that
claims to emit type A actually emits type B, and especially not to
do so as late as executor startup. I speculated in the other thread
that we could do so during planning instead, but that turns out to
just move the problems around. I think this must be so, because the
whole idea is bogus. For example, if we have a function that is
declared to take type "ct", it can't be okay in general to pass it
type "record" instead. We've mistakenly thought that we could fuzz
this as long as the two types are physically compatible --- but how
can we think that the column names of a composite type aren't a
basic part of its definition?
So 0001 attached fixes this by revoking the decision to apply
ExecTypeSetColNames in cases where a Var or RowExpr is declared
to return a named composite type. This causes a couple of regression
test results to change, but they are ones that were specifically
added to exercise this behavior that we now see to be invalid.
(In passing, this also adjusts ExecEvalWholeRowVar to fail if the
Var claims to be of a domain-over-composite. I'm not sure what
I was thinking when I changed it to allow that; the case should
not arise, and if it did, it'd be questionable because we're not
checking domain constraints here.)
0002 is some inessential mop-up that avoids storing useless column
name lists in RowExprs where they won't be used.
I'm not really thrilled about back-patching a behavioral change
of this sort, but I don't see any good alternative. Corrupting
people's tables is not okay.
Thoughts?
regards, tom lane
[1]
https://www.postgresql.org/message-id/flat/CAOFAq3BeawPiw9pc3bVGZ%3DRint2txWEBCeDC2wNAhtCZoo2ZqA%40mail.gmail.com
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 892b4e17e0..f0f3b4ffb0 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -1913,16 +1913,16 @@ ExecInitExprRec(Expr *node, ExprState *state,
{
/* generic record, use types of given expressions */
tupdesc = ExecTypeFromExprList(rowexpr->args);
+ /* ... but adopt RowExpr's column aliases */
+ ExecTypeSetColNames(tupdesc, rowexpr->colnames);
+ /* Bless the tupdesc so it can be looked up later */
+ BlessTupleDesc(tupdesc);
}
else
{
/* it's been cast to a named type, use that */
tupdesc = lookup_rowtype_tupdesc_copy(rowexpr->row_typeid, -1);
}
-/* In either case, adopt RowExpr's column aliases */
-ExecTypeSetColNames(tupdesc, rowexpr->colnames);
-/* Bless the tupdesc in case it's now of type RECORD */
-BlessTupleDesc(tupdesc);
/*
* In the named-type case, the tupdesc could have more columns
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index eb49817cee..b7b79e0b75 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -4021,12 +4021,8 @@ ExecEvalWholeRowVar(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
* generates an INT4 NULL regardless of the dropped column type).
* If we find a dropped column and cannot verify that case (1)
* holds, we have to use the slow path to check (2) for each row.
- *
- * If vartype is a domain over composite, just look through that
- * to the base composite type.
*/
- var_tupdesc = lookup_rowtype_tupdesc_domain(variable->vartype,
- -1, false);
+ var_tupdesc = lookup_rowtype_tupdesc(variable->vartype, -1);
slot_tupdesc = slot->tts_tupleDescriptor;
@@ -4063,9 +4059,8 @@ ExecEvalWholeRowVar(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
/*
* Use the variable's declared rowtype as the descriptor for the
- * output values, modulo possibly assigning new column names
- * below. In particular, we *must* absorb an
Re: enable certain TAP tests for MSVC builds
On Sun, Dec 05, 2021 at 11:57:31AM -0500, Andrew Dunstan wrote:
> Certain TAP tests rely on settings that the Make files provide for them.
> However vcregress.pl doesn't provide those settings. This patch remedies
> that, and I propose to apply it shortly (when we have a fix for the SSL
> tests that I will write about separately) and backpatch it appropriately.
> --- a/src/tools/msvc/vcregress.pl
> +++ b/src/tools/msvc/vcregress.pl
> @@ -59,6 +59,12 @@ copy("$Config/autoinc/autoinc.dll",
> "src/test/regress");
> copy("$Config/regress/regress.dll", "src/test/regress");
> copy("$Config/dummy_seclabel/dummy_seclabel.dll", "src/test/regress");
>
> +# Settings used by TAP tests
> +$ENV{with_ssl} = $config->{openssl} ? 'openssl' : 'no';
> +$ENV{with_ldap} = $config->{ldap} ? 'yes' : 'no';
> +$ENV{with_icu} = $config->{icu} ? 'yes' : 'no';
> +$ENV{with_gssapi} = $config->{gss} ? 'yes' : 'no';
That's appropriate. There are more variables to cover:
$ git grep -n ^export ':(glob)**/Makefile'
src/bin/pg_basebackup/Makefile:22:export LZ4
src/bin/pg_basebackup/Makefile:23:export TAR
src/bin/pg_basebackup/Makefile:27:export GZIP_PROGRAM=$(GZIP)
src/bin/psql/Makefile:20:export with_readline
src/test/kerberos/Makefile:16:export with_gssapi with_krb_srvnam
src/test/ldap/Makefile:16:export with_ldap
src/test/modules/ssl_passphrase_callback/Makefile:3:export with_ssl
src/test/recovery/Makefile:20:export REGRESS_SHLIB
src/test/ssl/Makefile:18:export with_ssl
src/test/subscription/Makefile:18:export with_icu
Re: The "char" type versus non-ASCII characters
Chapman Flack writes: > On 12/05/21 12:01, Tom Lane wrote: >> regression=# select '\'::bytea; >> ERROR: invalid input syntax for type bytea >> >> which would be incompatible with "char"'s existing behavior. But as >> long as we don't do that, I'd be okay with having high-bit-set char >> values map to backslash-followed-by-three-octal-digits, which is >> what bytea escape format would produce. > Is that a proposal to change nothing about the current treatment > of values < 128, or just to avoid rejecting bare '\'? I intended to change nothing about charin's treatment of ASCII characters, nor anything about bytea's behavior. I don't think we should relax the error checks in the latter. That does mean that backslash becomes a problem for the idea of transparent conversion from char to bytea or vice versa. We could think about emitting backslash as '\\' in charout, I suppose. I'm not really convinced though that bytea compatibility is worth changing a case that's non-problematic today. > If there's a way to factor out and reuse the good parts of byteain, > that would mean '\\' would also be accepted to mean a backslash, > and the \r \n \t usual escapes would be accepted too, and \ooo and > \xhh. Uh, what? regression=# select '\n'::bytea; ERROR: invalid input syntax for type bytea But I doubt that sharing code here would be worth the trouble. The vast majority of byteain is concerned with managing the string length, which is a nonissue for charin. > I think it ends up being no more complexity at all, because a single > octet in bytea-hex form looks like \xhh, which is exactly what > a single \xhh in bytea-escape form looks like. I'm confused by this statement too. AFAIK the alternatives in bytea are \xhh or \ooo: regression=# select '\xEE'::bytea; bytea --- \xee (1 row) regression=# set bytea_output to escape; SET regression=# select '\xEE'::bytea; bytea --- \356 (1 row) regards, tom lane
Re: MSVC SSL test failure
> On 5 Dec 2021, at 18:03, Andrew Dunstan wrote: > I am getting this test failure 001_ssltests.pl on my test MSVC system > when SSL tests are enabled: > >not ok 110 - certificate authorization fails with revoked client cert with > server-side CRL directory: matches > ># Failed test 'certificate authorization fails with revoked client cert > with server-side CRL directory: matches' ># at t/001_ssltests.pl line 618. ># 'psql: error: connection to server at "127.0.0.1", > port 59491 failed: server closed the connection unexpectedly ># This probably means the server terminated abnormally ># before or while processing the request. ># server closed the connection unexpectedly ># This probably means the server terminated abnormally ># before or while processing the request. ># server closed the connection unexpectedly ># This probably means the server terminated abnormally ># before or while processing the request.' ># doesn't match '(?^:SSL error: sslv3 alert certificate revoked)' > > There's nothing terribly suspicious in the server log, so I'm not quite > sure what's going on here. Trying out HEAD as of 20 minutes ago in Andres' MSVC setup on Cirrus CI I'm unable to replicate this. Did you remake the keys/certs or are they the files from the repo? Which version of OpenSSL is this using? -- Daniel Gustafsson https://vmware.com/
Re: [PATCH] Don't block HOT update by BRIN index
Justin Pryzby writes: > brin.sql's new brin_hot test is failing sometimes. > Evidently because: > | 2021-12-01 04:02:01.096 CET [61a6e587.3106b1:4] LOG: wait_for_hot_stats > delayed 33.217301 seconds > It seems like maybe the UDP packet lost to the stats collector got lost ? > It fails less than 10% of the time here, probably depending on load. Oh, geez. *Please* let us not add another regression failure mode like the ones that afflict stats.sql. We do not need a doubling of that failure rate. I suggest just removing this test. regards, tom lane
Re: The "char" type versus non-ASCII characters
On 12/05/21 14:51, Tom Lane wrote: > Uh, what? > > regression=# select '\n'::bytea; > ERROR: invalid input syntax for type bytea Wow, I was completely out to lunch there somehow. Sorry. None of those other escaped forms are known to byteain, other than '\\' and according to table 8.7. I can't even explain why I thought that. > I'm confused by this statement too. AFAIK the alternatives in > bytea are \xhh or \ooo: Here I think I can at least tell where I went wrong; I saw both an octal and a hex column in table 8.7, which I saw located under the "bytea escape format" heading, and without testing carefully enough, I assumed it was telling me that either format would be recognized on input, which would certainly be possible, but clearly I was carrying over too many assumptions from other escape formats where I'm used to that being the case. If I wanted to prevent another reader making my exact mistake, I might re-title those two table columns to be "In bytea escape format" and "In bytea hex format" to make it more clear the table is combining information for both formats. I'm sure I did test SELECT '\x41'::bytea, but that proved nothing, being simply interpreted as the hex input format. I should have tried SELECT 'A\x41'::bytea, and would have immediately seen it rejected. I've just looked at datatypes.sgml, where I was expecting to see that table 8.7 actually falls outside of the sect2 for "bytea escape format", and that I had simply misinterpreted it because the structural nesting isn't obvious in the rendered HTML. But what I found was that the table actually /is/ nested inside the "bytea escape format" section, and in the generated HTML it is within the div for that section, and the table's own div has the ID DATATYPE-BINARY-SQLESC. The change history there appears complex. The table already existed at the time of a2a8c7a, which made a "bytea escape format" sect2 out of the existing text that included the table, and added a separate "bytea hex format" sect2. But the table at that point showed only the input and output representations for the escape format, didn't say anything about hex format, and wasn't touched in that commit. Nine years later, f77de4b changed the values in the rightmost column to hex form, but only because that was then the "output representation" column and the default output format had been changed to hex. Five months after that, f10a20e changed the heading of that column from "output representation" to "hex representation", probably because the values in that column by then were hex. So it ended up as a table that is structurally part of the "bytea escape format" section, whose rightmost column shows a hex format, and therefore (ahem) could suggest to a reader (who doesn't rush to psql and test it thoroughly) that a hex format is accepted there. Still, I could have avoided that if I had better tested my reading first. Regards, -Chap
Re: MSVC SSL test failure
On 12/5/21 15:14, Daniel Gustafsson wrote: >> On 5 Dec 2021, at 18:03, Andrew Dunstan wrote: >> I am getting this test failure 001_ssltests.pl on my test MSVC system >> when SSL tests are enabled: >> >>not ok 110 - certificate authorization fails with revoked client cert >> with server-side CRL directory: matches >> >># Failed test 'certificate authorization fails with revoked client cert >> with server-side CRL directory: matches' >># at t/001_ssltests.pl line 618. >># 'psql: error: connection to server at "127.0.0.1", >> port 59491 failed: server closed the connection unexpectedly >># This probably means the server terminated abnormally >># before or while processing the request. >># server closed the connection unexpectedly >># This probably means the server terminated abnormally >># before or while processing the request. >># server closed the connection unexpectedly >># This probably means the server terminated abnormally >># before or while processing the request.' >># doesn't match '(?^:SSL error: sslv3 alert certificate revoked)' >> >> There's nothing terribly suspicious in the server log, so I'm not quite >> sure what's going on here. > Trying out HEAD as of 20 minutes ago in Andres' MSVC setup on Cirrus CI I'm > unable to replicate this. Did you remake the keys/certs or are they the files > from the repo? Which version of OpenSSL is this using? > It's WS2019 Build 1809, VS2019, openssl 1.1.1 (via chocolatey). Can you show me the cirrus.yml file you're using to test with? A URL ref to the results would also help. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: enable certain TAP tests for MSVC builds
On 12/5/21 14:47, Noah Misch wrote:
> On Sun, Dec 05, 2021 at 11:57:31AM -0500, Andrew Dunstan wrote:
>> Certain TAP tests rely on settings that the Make files provide for them.
>> However vcregress.pl doesn't provide those settings. This patch remedies
>> that, and I propose to apply it shortly (when we have a fix for the SSL
>> tests that I will write about separately) and backpatch it appropriately.
>> --- a/src/tools/msvc/vcregress.pl
>> +++ b/src/tools/msvc/vcregress.pl
>> @@ -59,6 +59,12 @@ copy("$Config/autoinc/autoinc.dll",
>> "src/test/regress");
>> copy("$Config/regress/regress.dll", "src/test/regress");
>> copy("$Config/dummy_seclabel/dummy_seclabel.dll", "src/test/regress");
>>
>> +# Settings used by TAP tests
>> +$ENV{with_ssl} = $config->{openssl} ? 'openssl' : 'no';
>> +$ENV{with_ldap} = $config->{ldap} ? 'yes' : 'no';
>> +$ENV{with_icu} = $config->{icu} ? 'yes' : 'no';
>> +$ENV{with_gssapi} = $config->{gss} ? 'yes' : 'no';
> That's appropriate. There are more variables to cover:
>
> $ git grep -n ^export ':(glob)**/Makefile'
> src/bin/pg_basebackup/Makefile:22:export LZ4
> src/bin/pg_basebackup/Makefile:23:export TAR
> src/bin/pg_basebackup/Makefile:27:export GZIP_PROGRAM=$(GZIP)
> src/bin/psql/Makefile:20:export with_readline
> src/test/kerberos/Makefile:16:export with_gssapi with_krb_srvnam
> src/test/ldap/Makefile:16:export with_ldap
> src/test/modules/ssl_passphrase_callback/Makefile:3:export with_ssl
> src/test/recovery/Makefile:20:export REGRESS_SHLIB
> src/test/ssl/Makefile:18:export with_ssl
> src/test/subscription/Makefile:18:export with_icu
LZ4/TAR/GZIP_PROGAM: It's not clear what these should be set to. The TAP
tests skip tests that use them if they are not set.
with_readline: we don't build with readline on Windows, period. I guess
we could just set it to "no".
REGRESS_SHLIB: already set in vcregress.pl
with_krb_srvnam: the default is "postgres", we could just set it to that
I guess.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: MSVC SSL test failure
> On 5 Dec 2021, at 23:44, Andrew Dunstan wrote: > Can you show me the cirrus.yml file you're using to test with? I used the 0001 patch from this thread: https://www.postgresql.org/message-id/20211101055720.7mzwtkhzxmorpxth%40alap3.anarazel.de > A URL ref to the results would also help. I can't vouch for how helpful it is, but below is the build info that exists: https://cirrus-ci.com/task/5358152892809216 -- Daniel Gustafsson https://vmware.com/
Re: enable certain TAP tests for MSVC builds
On Sun, Dec 05, 2021 at 06:00:08PM -0500, Andrew Dunstan wrote:
> On 12/5/21 14:47, Noah Misch wrote:
> > On Sun, Dec 05, 2021 at 11:57:31AM -0500, Andrew Dunstan wrote:
> >> Certain TAP tests rely on settings that the Make files provide for them.
> >> However vcregress.pl doesn't provide those settings. This patch remedies
> >> that, and I propose to apply it shortly (when we have a fix for the SSL
> >> tests that I will write about separately) and backpatch it appropriately.
> >> --- a/src/tools/msvc/vcregress.pl
> >> +++ b/src/tools/msvc/vcregress.pl
> >> @@ -59,6 +59,12 @@ copy("$Config/autoinc/autoinc.dll",
> >> "src/test/regress");
> >> copy("$Config/regress/regress.dll", "src/test/regress");
> >> copy("$Config/dummy_seclabel/dummy_seclabel.dll", "src/test/regress");
> >>
> >> +# Settings used by TAP tests
> >> +$ENV{with_ssl} = $config->{openssl} ? 'openssl' : 'no';
> >> +$ENV{with_ldap} = $config->{ldap} ? 'yes' : 'no';
> >> +$ENV{with_icu} = $config->{icu} ? 'yes' : 'no';
> >> +$ENV{with_gssapi} = $config->{gss} ? 'yes' : 'no';
> > That's appropriate. There are more variables to cover:
> >
> > $ git grep -n ^export ':(glob)**/Makefile'
> > src/bin/pg_basebackup/Makefile:22:export LZ4
> > src/bin/pg_basebackup/Makefile:23:export TAR
> > src/bin/pg_basebackup/Makefile:27:export GZIP_PROGRAM=$(GZIP)
> > src/bin/psql/Makefile:20:export with_readline
> > src/test/kerberos/Makefile:16:export with_gssapi with_krb_srvnam
> > src/test/ldap/Makefile:16:export with_ldap
> > src/test/modules/ssl_passphrase_callback/Makefile:3:export with_ssl
> > src/test/recovery/Makefile:20:export REGRESS_SHLIB
> > src/test/ssl/Makefile:18:export with_ssl
> > src/test/subscription/Makefile:18:export with_icu
>
> LZ4/TAR/GZIP_PROGAM: It's not clear what these should be set to. The TAP
> tests skip tests that use them if they are not set.
Could add config.pl entries for those. Preventing those skips on Windows may
or may not be worth making config.pl readers think about them.
> with_readline: we don't build with readline on Windows, period. I guess
> we could just set it to "no".
> with_krb_srvnam: the default is "postgres", we could just set it to that
> I guess.
Works for me.
Re: parse_subscription_options - suggested improvements
On Fri, Dec 3, 2021 at 11:02 AM Bossart, Nathan wrote:
>
> On 8/8/21, 11:54 PM, "Peter Smith" wrote:
> > v11 -> v12
> >
> > * A rebase was needed due to some recent pgindent changes on HEAD.
>
> The patch looks correct to me. I have a couple of small comments.
Thank you for taking an interest in my patch and moving it to a
"Ready" state in the CF.
>
> + /* Start out with cleared opts. */
> + memset(opts, 0, sizeof(SubOpts));
>
> Should we stop initializing opts in the callers?
For the initialization of opts I put memset within the function to
make it explicit that the bit-masks will work as intended without
having to look back at calling code for the initial values. In any
case, I think the caller declarations of SubOpts are trivial, (e.g.
SubOpts opts = {0};) so I felt caller initializations don't need to be
changed regardless of the memset.
>
> - if (opts->enabled &&
> - IsSet(supported_opts, SUBOPT_ENABLED) &&
> - !IsSet(opts->specified_opts, SUBOPT_ENABLED))
> + if (opts->enabled)
> ereport(ERROR,
> (errcode(ERRCODE_SYNTAX_ERROR),
> /*- translator: both %s are strings of the form
> "option = value" */
> errmsg("subscription with %s must
> also set %s",
> "slot_name = NONE",
> "enabled = false")));
>
> IMO the way these 'if' statements are written isn't super readable.
> Right now, it's written like this:
>
> if (opt && IsSet(someopt))
> ereport(ERROR, ...);
>
> if (otheropt && IsSet(someotheropt))
> ereport(ERROR, ...);
>
> if (opt)
> ereport(ERROR, ...);
>
> if (otheropt)
> ereport(ERROR, ...);
>
> I think it would be easier to understand if it was written more like
> this:
>
> if (opt)
> {
> if (IsSet(someopt))
> ereport(ERROR, ...);
> else
> ereport(ERROR, ...);
> }
>
> if (otheropt)
> {
> if (IsSet(someotheropt))
> ereport(ERROR, ...);
> else
> ereport(ERROR, ...);
> }
>
> Of course, this does result in a small behaviour change because the
> order of the checks is different, but I'm not sure that's a big deal.
> Ultimately, it would probably be nice to report all the errors instead
> of just the first one that is hit, but again, I don't know if that's
> worth the effort.
>
My patch was meant only to remove all the redundant conditions of the
HEAD code, so I did not rearrange any of the logic at all. Personally,
I also think your v13 is better and easier to read, but those subtle
behaviour differences were something I'd deliberately avoided in v12.
However, if the committer thinks it does not matter then your v13 is
fine by me.
> I attached a new version of the patch with my suggestions. However, I
> think v12 is committable.
>
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
Re: pg_dump versus ancient server versions
I ran a new set of experiments concerning building back branches on modern platforms, this time trying Fedora 35 (gcc 11.2.1) on x86_64. I widened the scope of the testing a bit by adding "--enable-nls --with-perl" and running check-world not just the core tests. Salient results: * Everything back to 9.2 passes the test, although with more and more compile warnings the further back you go. * 9.1 fails with "conflicting types for 'base_yylex'", much as I saw on macOS except it's a hard error on this compiler. * Parallel check-world is pretty unreliable before v10 (I knew this already, actually). But without parallelism, it's fine. Based on these results, I think maybe we should raise our ambitions a bit compared to Peter's original proposal. Specifically, I wonder if it wouldn't be wise to try to silence compile warnings in these branches. The argument for this is basically that if we don't, then every time someone builds one of these branches, they have to tediously go through the warnings and verify that they're not important. It won't take long for the accumulated time-wastage from that to exceed the cost of back-patching whatever we did to silence the warning in later branches. Now, I'm still not interested in trying to silence maybe-uninitialized warnings pre-9.3, mainly because of the ereport-ERROR-doesnt-return issue. (I saw far fewer of those under gcc than clang, but not zero.) We could ignore those figuring that 9.2 will be out of scope in a year anyway, or else teach 9.2's configure to select -Wno-maybe-uninitialized where possible. Likewise, getting check-world to parallelize successfully pre-v10 seems like a bridge too far. But I would, for example, be in favor of back-patching eb9812f27 (Make pg_upgrade's test.sh less chatty). It's just annoying to run check-world and get that output now. regards, tom lane
Re: [PATCH] Don't block HOT update by BRIN index
On 12/5/21 21:16, Tom Lane wrote: Justin Pryzby writes: brin.sql's new brin_hot test is failing sometimes. Evidently because: | 2021-12-01 04:02:01.096 CET [61a6e587.3106b1:4] LOG: wait_for_hot_stats delayed 33.217301 seconds It seems like maybe the UDP packet lost to the stats collector got lost ? It fails less than 10% of the time here, probably depending on load. Oh, geez. *Please* let us not add another regression failure mode like the ones that afflict stats.sql. We do not need a doubling of that failure rate. I suggest just removing this test. Whooops. Agreed, I'll get rid of that test. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Is ssl_crl_file "SSL server cert revocation list"?
At Fri, 3 Dec 2021 14:32:54 +0100, Daniel Gustafsson wrote in > > On 2 Dec 2021, at 16:04, Peter Eisentraut > > wrote: > > > This change looks correct to me. > > Thanks for review, I've pushed this backpatched (in part) down to 10. Thanks for revising and comitting this. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: On login trigger: take three
On Tue, Nov 16, 2021 at 4:38 PM Greg Nancarrow wrote: > > I've attached an updated patch with these changes. > I've attached a re-based version (no functional changes from the previous) to fix cfbot failures. Regards, Greg Nancarrow Fujitsu Australia v23-0001-Add-a-new-login-event-and-login-event-trigger-support.patch Description: Binary data
Re: [PATCH] Don't block HOT update by BRIN index
Tomas Vondra writes: > On 12/5/21 21:16, Tom Lane wrote: >> Oh, geez. *Please* let us not add another regression failure mode >> like the ones that afflict stats.sql. We do not need a doubling >> of that failure rate. I suggest just removing this test. > Whooops. Agreed, I'll get rid of that test. Another idea, perhaps, is to shove that test into stats.sql, where people would know to ignore it? (Actually, I've thought more than once that we should mark stats.sql as ignorable in the schedule ...) regards, tom lane
Re: MSVC SSL test failure
On 12/5/21 12:50, Tom Lane wrote: > Andrew Dunstan writes: >> I am getting this test failure 001_ssltests.pl on my test MSVC system >> when SSL tests are enabled: >> not ok 110 - certificate authorization fails with revoked client cert >> with server-side CRL directory: matches >> # Failed test 'certificate authorization fails with revoked client >> cert with server-side CRL directory: matches' >> # at t/001_ssltests.pl line 618. >> # 'psql: error: connection to server at "127.0.0.1", >> port 59491 failed: server closed the connection unexpectedly >> # This probably means the server terminated abnormally >> # before or while processing the request. >> # server closed the connection unexpectedly >> # This probably means the server terminated abnormally >> # before or while processing the request. >> # server closed the connection unexpectedly >> # This probably means the server terminated abnormally >> # before or while processing the request.' >> # doesn't match '(?^:SSL error: sslv3 alert certificate revoked)' >> There's nothing terribly suspicious in the server log, so I'm not quite >> sure what's going on here. > Hmm .. does enabling log_connections/log_disconnections produce > any useful info? No, that's already turned on (this test is run using a standard buildfarm client). > > This looks quite a bit like the sort of failure that commit > 6051857fc was meant to forestall. I wonder whether reverting > that commit changes the results? You might also try inserting > a shutdown() call, as we'd decided not to do [1]. > > regards, tom lane > > [1] https://www.postgresql.org/message-id/1283317.1638213407%40sss.pgh.pa.us Commenting out the closesocket() worked. I will look at shutdown(). cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints
On Fri, Dec 3, 2021 at 8:28 PM Dilip Kumar wrote: > On Fri, Dec 3, 2021 at 7:38 PM Ashutosh Sharma > wrote: > > > > I see that this patch is reducing the database creation time by almost > 3-4 times provided that the template database has some user data in it. > However, there are couple of points to be noted: > > Thanks a lot for looking into the patches. > > > > 1) It makes the crash recovery a bit slower than before if the crash has > occurred after the execution of a create database statement. Moreover, if > the template database size is big, it might even generate a lot of WAL > files which the user needs to be aware of. > > Yes it will but actually that is the only correct way to do it, in > current we are just logging the WAL as copying the source directory to > destination directory without really noting down exactly what we > wanted to copy, so we are force to do the checkpoint right after > create database because in crash recovery we can not actually replay > that WAL. Because WAL just say copy the source to destination so it > is very much possible that at the DO time source directory had some > different content than the REDO time so this would have created the > inconsistencies in the crash recovery so to avoid this bug they force > the checkpoint so now also if you do force checkpoint then again crash > recovery will be equally fast. So I would not say that we have made > crash recovery slow but we have removed some bugs and with that now we > don't need to force the checkpoint. Also note that in current code > even with force checkpoint the bug is not completely avoided in all > the cases, check below comments from the code[1]. > > > 2) This will put a lot of load on the first checkpoint that will occur > after creating the database statement. I will experiment around this to see > if this has any side effects. > > But now a checkpoint can happen at its own need and there is no need > to force a checkpoint like it was before patch. > > So the major goal of this patch is 1) Correctly WAL log the create > database which is hack in the current system, 2) Avoid force > checkpoints, 3) We copy page by page so it will support TDE because if > the source and destination database has different encryption then we > can reencrypt the page before copying to destination database, which > is not possible in current system as we are copying directory 4) Now > the new database pages will get the latest LSN which is the correct > things earlier new database pages were getting copied directly with > old LSN only. > OK. Understood, thanks.! -- With Regards, Ashutosh Sharma.
Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints
Here are few more review comments:
1) It seems that we are not freeing the memory allocated for buf.data in
CreateDirAndVersionFile().
--
+ */
+static void
+CreateDirAndVersionFile(char *dbpath, Oid dbid, Oid tsid, bool isRedo)
+{
2) Do we need to pass dbpath here? I mean why not reconstruct it from dbid
and tsid.
--
3) Not sure if this point has already been discussed, Will we be able to
recover the data when wal_level is set to minimal because the following
condition would be false with this wal level.
+ use_wal = XLogIsNeeded() &&
+ (relpersistence == RELPERSISTENCE_PERMANENT || copying_initfork);
--
With Regards,
Ashutosh Sharma.
On Mon, Dec 6, 2021 at 9:12 AM Ashutosh Sharma
wrote:
> On Fri, Dec 3, 2021 at 8:28 PM Dilip Kumar wrote:
>
>> On Fri, Dec 3, 2021 at 7:38 PM Ashutosh Sharma
>> wrote:
>> >
>> > I see that this patch is reducing the database creation time by almost
>> 3-4 times provided that the template database has some user data in it.
>> However, there are couple of points to be noted:
>>
>> Thanks a lot for looking into the patches.
>> >
>> > 1) It makes the crash recovery a bit slower than before if the crash
>> has occurred after the execution of a create database statement. Moreover,
>> if the template database size is big, it might even generate a lot of WAL
>> files which the user needs to be aware of.
>>
>> Yes it will but actually that is the only correct way to do it, in
>> current we are just logging the WAL as copying the source directory to
>> destination directory without really noting down exactly what we
>> wanted to copy, so we are force to do the checkpoint right after
>> create database because in crash recovery we can not actually replay
>> that WAL. Because WAL just say copy the source to destination so it
>> is very much possible that at the DO time source directory had some
>> different content than the REDO time so this would have created the
>> inconsistencies in the crash recovery so to avoid this bug they force
>> the checkpoint so now also if you do force checkpoint then again crash
>> recovery will be equally fast. So I would not say that we have made
>> crash recovery slow but we have removed some bugs and with that now we
>> don't need to force the checkpoint. Also note that in current code
>> even with force checkpoint the bug is not completely avoided in all
>> the cases, check below comments from the code[1].
>>
>> > 2) This will put a lot of load on the first checkpoint that will occur
>> after creating the database statement. I will experiment around this to see
>> if this has any side effects.
>>
>> But now a checkpoint can happen at its own need and there is no need
>> to force a checkpoint like it was before patch.
>>
>> So the major goal of this patch is 1) Correctly WAL log the create
>> database which is hack in the current system, 2) Avoid force
>> checkpoints, 3) We copy page by page so it will support TDE because if
>> the source and destination database has different encryption then we
>> can reencrypt the page before copying to destination database, which
>> is not possible in current system as we are copying directory 4) Now
>> the new database pages will get the latest LSN which is the correct
>> things earlier new database pages were getting copied directly with
>> old LSN only.
>>
>
> OK. Understood, thanks.!
>
> --
> With Regards,
> Ashutosh Sharma.
>
Re: Optionally automatically disable logical replication subscriptions on error
On Sat, Dec 4, 2021 at 12:20 AM osumi.takami...@fujitsu.com
wrote:
>
> Hi, I've made a new patch v11 that incorporated suggestions described above.
>
Some review comments for the v11 patch:
doc/src/sgml/ref/create_subscription.sgml
(1) Possible wording improvement?
BEFORE:
+ Specifies whether the subscription should be automatically disabled
+ if replicating data from the publisher triggers errors. The default
+ is false.
AFTER:
+ Specifies whether the subscription should be automatically disabled
+ if any errors are detected by subscription workers during data
+ replication from the publisher. The default is false.
src/backend/replication/logical/worker.c
(2) WorkerErrorRecovery comments
Instead of:
+ * As a preparation for disabling the subscription, emit the error,
+ * handle the transaction and clean up the memory context of
+ * error. ErrorContext is reset by FlushErrorState.
why not just say:
+ Worker error recovery processing, in preparation for disabling the
+ subscription.
And then comment the function's code lines:
e.g.
/* Emit the error */
...
/* Abort any active transaction */
...
/* Reset the ErrorContext */
...
(3) DisableSubscriptionOnError return
The "if (!subform->subdisableonerr)" block should probably first:
heap_freetuple(tup);
(regardless of the fact the only current caller will proc_exit anyway)
(4) did_error flag
I think perhaps the previously-used flag name "disable_subscription"
is better, or maybe "error_recovery_done".
Also, I think it would look better if it was set AFTER
WorkerErrorRecovery() was called.
(5) DisableSubscriptionOnError LOG message
This version of the patch removes the LOG message:
+ ereport(LOG,
+ errmsg("logical replication subscription \"%s\" will be disabled due
to error: %s",
+MySubscription->name, edata->message));
Perhaps a similar error message could be logged prior to EmitErrorReport()?
e.g.
"logical replication subscription \"%s\" will be disabled due to an error"
Regards,
Greg Nancarrow
Fujitsu Australia
Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints
On Mon, Dec 6, 2021 at 9:17 AM Ashutosh Sharma wrote:
>
> Here are few more review comments:
Thanks for reviewing it.
> 1) It seems that we are not freeing the memory allocated for buf.data in
> CreateDirAndVersionFile().
Yeah this was a problem in v6 but I have fixed in v7, can you check that.
>
> + */
> +static void
> +CreateDirAndVersionFile(char *dbpath, Oid dbid, Oid tsid, bool isRedo)
> +{
>
> 2) Do we need to pass dbpath here? I mean why not reconstruct it from dbid
> and tsid.
Yeah we can do that but I thought computing dbpath has some cost and
since the caller already has it why not to pass it.
>
> 3) Not sure if this point has already been discussed, Will we be able to
> recover the data when wal_level is set to minimal because the following
> condition would be false with this wal level.
>
> + use_wal = XLogIsNeeded() &&
> + (relpersistence == RELPERSISTENCE_PERMANENT || copying_initfork);
>
Since we are creating new relfilenode this is fine, refer "Skipping
WAL for New RelFileNode" in src/backend/access/transam/README
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Disallow quorum uncommitted (with synchronous standbys) txns in logical replication subscribers
Hi, It looks like the logical replication subscribers are receiving the quorum uncommitted transactions even before the synchronous (sync) standbys. Most of the times it is okay, but it can be a problem if the primary goes down/crashes (while the primary is in SyncRepWaitForLSN) before the quorum commit is achieved (i.e. before the sync standbys receive the committed txns from the primary) and the failover is to happen on to the sync standby. The subscriber would have received the quorum uncommitted txns whereas the sync standbys didn't. After the failover, the new primary (the old sync standby) would be behind the subscriber i.e. the subscriber will be seeing the data that the new primary can't. Is there a way the subscriber can get back to be in sync with the new primary? In other words, can we reverse the effects of the quorum uncommitted txns on the subscriber? Naive way is to do it manually, but it doesn't seem to be elegant. We have performed a small experiment to observe the above behaviour with 1 primary, 1 sync standby and 1 subscriber: 1) Have a wait loop in SyncRepWaitForLSN (a temporary hack to illustrate the standby receiving the txn a bit late or fail to receive) 2) Insert data into a table on the primary 3) The primary waits i.e. the insert query hangs (because of the wait loop hack ()) before the local txn is sent to the sync standby, whereas the subscriber receives the inserted data. 4) If the primary crashes/goes down and unable to come up, if the failover happens to sync standby (which didn't receive the data that got inserted on tbe primary), the subscriber would see the data that the sync standby can't. This looks to be a problem. A possible solution is to let the subscribers receive the txns only after the primary achieves quorum commit (gets out of the SyncRepWaitForLSN or after all sync standbys received the txns). The logical replication walsenders can wait until the quorum commit is obtained and then can send the WAL. A new GUC can be introduced to control this, default being the current behaviour. Thoughts? Thanks Satya (cc-ed) for the use-case and off-list discussion. Regards, Bharath Rupireddy.
Re: Optionally automatically disable logical replication subscriptions on error
> On Dec 1, 2021, at 8:48 PM, Amit Kapila wrote: > > The patch disables the subscription for non-transient errors. I am not > sure if we can easily make the call to decide whether any particular > error is transient or not. For example, DISK_FULL or OUT_OF_MEMORY > might not rectify itself. Why not just allow to disable the > subscription on any error? And then let the user check the error > either in view or logs and decide whether it would like to enable the > subscription or do something before it (like making space in disk, or > fixing the network). The original idea of the patch, back when I first wrote and proposed it, was to remove the *absurdity* of retrying a transaction which, in the absence of human intervention, was guaranteed to simply fail again ad infinitum. Retrying in the face of resource errors is not *absurd* even though it might fail again ad infinitum. The reason is that there is at least a chance that the situation will clear up without human intervention. > The other problem I see with this transient error stuff is maintaining > the list of error codes that we think are transient. I think we need a > discussion for each of the error_codes we are listing now and whatever > new error_code we add in the future which doesn't seem like a good > idea. A reasonable rule might be: "the subscription will be disabled if the server can determine that retries cannot possibly succeed without human intervention." We shouldn't need to categorize all error codes perfectly, as long as we're conservative. What I propose is similar to how we determine whether to mark a function leakproof; we don't have to mark all leakproof functions as such, we just can't mark one as such if it is not. If we're going to debate the error codes, I think we would start with an empty list, and add to the list on sufficient analysis. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce)
On Tue, Oct 26, 2021 at 6:55 PM Shruthi Gowda wrote:
>
>
> I have revised the patch w.r.t the way 'create_storage' is interpreted
> in heap_create() along with some minor changes to preserve the DBOID
> patch.
>
Hi Shruthi,
I am reviewing the attached patches and providing a few comments here
below for patch "v5-0002-Preserve-database-OIDs-in-pg_upgrade.patch"
1.
--- a/doc/src/sgml/ref/create_database.sgml
+++ b/doc/src/sgml/ref/create_database.sgml
@@ -31,7 +31,8 @@ CREATE DATABASE name
- [ IS_TEMPLATE [=] istemplate ] ]
+ [ IS_TEMPLATE [=] istemplate ]
+ [ OID [=] db_oid ] ]
Replace "db_oid" with 'oid'. Below in the listitem, we have mentioned 'oid'.
2.
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
+ if ((dboid < FirstNormalObjectId) &&
+ (strcmp(dbname, "template0") != 0) &&
+ (!IsBinaryUpgrade))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE)),
+ errmsg("Invalid value for option \"%s\"", defel->defname),
+ errhint("The specified OID %u is less than the minimum OID for user
objects %u.",
+ dboid, FirstNormalObjectId));
+ }
Are we sure that 'IsBinaryUpgrade' will be set properly, before the
createdb function is called? Can we recheck once ?
3.
@@ -504,11 +525,15 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
*/
pg_database_rel = table_open(DatabaseRelationId, RowExclusiveLock);
- do
+ /* Select an OID for the new database if is not explicitly configured. */
+ if (!OidIsValid(dboid))
{
- dboid = GetNewOidWithIndex(pg_database_rel, DatabaseOidIndexId,
-Anum_pg_database_oid);
- } while (check_db_file_conflict(dboid));
I think we need to do 'check_db_file_conflict' for the USER given OID
also.. right? It may already be present.
4.
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
/*
+ * Create template0 database with oid Template0ObjectId i.e, 4
+ */
+
Better to mention here, why OID 4 is reserved for template0 database?.
5.
+ /*
+ * Create template0 database with oid Template0ObjectId i.e, 4
+ */
+ static const char *const template0_setup[] = {
+ "CREATE DATABASE template0 IS_TEMPLATE = true ALLOW_CONNECTIONS = false OID "
+ CppAsString2(Template0ObjectId) ";\n\n",
Can we write something like, 'OID = CppAsString2(Template0ObjectId)'?
mention "=".
6.
+
+ /*
+ * We use the OID of postgres to determine datlastsysoid
+ */
+ "UPDATE pg_database SET datlastsysoid = "
+ "(SELECT oid FROM pg_database "
+ "WHERE datname = 'postgres');\n\n",
+
Make the above comment a single line comment.
7.
There are some spelling mistakes in the comments as below, please
correct the same
+ /*
+ * Make sure that binary upgrade propogate the database OID to the
new => correct spelling
+ * cluster
+ */
+/* OID 4 is reserved for Templete0 database */
> Correct spelling
+#define Template0ObjectId 4
I am reviewing another patch
"v5-0001-Preserve-relfilenode-and-tablespace-OID-in-pg_upg" as well
and will provide the comments soon if any...
Thanks & Regards
SadhuPrasad
EnterpriseDB: http://www.enterprisedb.com
Re: parallel vacuum comments
On Fri, Dec 3, 2021 at 6:06 PM Masahiko Sawada wrote:
>
> On Fri, Dec 3, 2021 at 6:03 PM Amit Kapila wrote:
> >
> > On Thu, Dec 2, 2021 at 6:01 PM Masahiko Sawada
> > wrote:
> > >
> > > I've attached updated patches.
> > >
> >
> > I have a few comments on v4-0001.
>
> Thank you for the comments!
>
> > 1.
> > In parallel_vacuum_process_all_indexes(), we can combine the two
> > checks for vacuum/cleanup at the beginning of the function
>
> Agreed.
>
> > and I think
> > it is better to keep the variable name as bulkdel or cleanup instead
> > of vacuum as that is more specific and clear.
>
> I was thinking to use the terms "bulkdel" and "cleanup" instead of
> "vacuum" and "cleanup" for the same reason. That way, probably we can
> use “bulkdel" and “cleanup" when doing index bulk-deletion (i.g.,
> calling to ambulkdelete) and index cleanup (calling to
> amvacuumcleanup), respectively, and use "vacuum" when processing an
> index, i.g., doing either bulk-delete or cleanup, instead of using
> just "processing" . But we already use “vacuum” and “cleanup” in many
> places, e.g., lazy_vacuum_index() and lazy_cleanup_index(). If we want
> to use “bulkdel” instead of “vacuum”, I think it would be better to
> change the terminology in vacuumlazy.c thoroughly, probably in a
> separate patch.
>
Okay.
> > 2. The patch seems to be calling parallel_vacuum_should_skip_index
> > thrice even before starting parallel vacuum. It has a call to find the
> > number of blocks which has to be performed for each index. I
> > understand it might not be too costly to call this but it seems better
> > to remember this info like we are doing in the current code.
>
> Yes, we can bring will_vacuum_parallel array back to the code. That
> way, we can remove the call to parallel_vacuum_should_skip_index() in
> parallel_vacuum_begin().
>
> > We can
> > probably set parallel_workers_can_process in parallel_vacuum_begin and
> > then again update in parallel_vacuum_process_all_indexes. Won't doing
> > something like that be better?
>
> parallel_workers_can_process can vary depending on bulk-deletion, the
> first time cleanup, or the second time (or more) cleanup. What can we
> set parallel_workers_can_process based on in parallel_vacuum_begin()?
>
I was thinking to set the results of will_vacuum_parallel in
parallel_vacuum_begin().
> >
> > 3. /*
> > * Copy the index bulk-deletion result returned from ambulkdelete and
> > @@ -2940,19 +2935,20 @@ parallel_process_one_index(Relation indrel,
> > * Since all vacuum workers write the bulk-deletion result at different
> > * slots we can write them without locking.
> > */
> > - if (shared_istat && !shared_istat->updated && istat_res != NULL)
> > + if (!pindstats->istat_updated && istat_res != NULL)
> > {
> > - memcpy(&shared_istat->istat, istat_res, sizeof(IndexBulkDeleteResult));
> > - shared_istat->updated = true;
> > + memcpy(&(pindstats->istat), istat_res, sizeof(IndexBulkDeleteResult));
> > + pindstats->istat_updated = true;
> >
> > /* Free the locally-allocated bulk-deletion result */
> > pfree(istat_res);
> > -
> > - /* return the pointer to the result from shared memory */
> > - return &shared_istat->istat;
> > }
> >
> > I think here now we copy the results both for local and parallel
> > cleanup. Isn't it better to write something about that in comments as
> > it is not clear from current comments?
>
> What do you mean by "local cleanup"?
>
Clean-up performed by leader backend.
> >
> > 4.
> > + /* Estimate size for shared information -- PARALLEL_VACUUM_KEY_SHARED */
> > + est_shared_len = MAXALIGN(sizeof(LVShared));
> > shm_toc_estimate_chunk(&pcxt->estimator, est_shared_len);
> >
> > Do we need MAXALIGN here? I think previously we required it here
> > because immediately after this we were writing index stats but now
> > those are allocated separately. Normally, shm_toc_estimate_chunk()
> > aligns the size but sometimes we need to do it so as to avoid
> > unaligned accessing of shared mem. I am really not sure whether we
> > require it for dead_items, do you remember why it is there in the
> > first place.
>
> Indeed. I don't remember that. Probably it's an oversight.
>
Yeah, I also think so.
--
With Regards,
Amit Kapila.
Re: pg_get_publication_tables() output duplicate relid
On Fri, Dec 3, 2021 at 6:04 PM Amit Langote wrote: > > On Fri, Dec 3, 2021 at 12:37 PM Amit Kapila wrote: > > On Thu, Dec 2, 2021 at 7:18 PM Amit Langote wrote: > > > On Thu, Dec 2, 2021 at 2:27 PM Amit Kapila > > > wrote: > > > > On Thu, Dec 2, 2021 at 8:42 AM Amit Langote > > > > wrote: > > > > > Reading Alvaro's email at the top again gave me a pause to reconsider > > > > > what I had said in reply. It might indeed have been nice if the > > > > > publication DDL itself had prevented the cases where a partition and > > > > > its ancestor are added to a publication, though as Hou-san mentioned, > > > > > partition ATTACHes make that a bit tricky to enforce at all times, > > > > > though of course not impossible. Maybe it's okay to just de-duplicate > > > > > pg_publication_tables output as the patch does, even though it may > > > > > appear to be a band-aid solution if we one considers Alvaro's point > > > > > about consistency. > > > > > > > > True, I think even if we consider that idea it will be a much bigger > > > > change, and also as it will be a behavioral change so we might want to > > > > keep it just for HEAD and some of these fixes need to be backpatched. > > > > Having said that, if you or someone want to pursue that idea and come > > > > up with a better solution than what we have currently it is certainly > > > > welcome. > > > > > > Okay, I did write a PoC patch this morning after sending out my > > > earlier email. I polished it a bit, which is attached. > > > > I see multiple problems with this patch and idea. > > Thanks for looking at it. Yeah, I have not looked very closely at ALL > TABLES [IN SCHEMA], though only because I suspected that those cases > deal with partitioning in such a way that the partition duplication > issue doesn't arise. That is, only the FOR TABLE list_of_tables and > ADD TABLE syntax allow for the duplication issue to occur. > > > (a) I think you > > forgot to deal with "All Tables In Schema" Publication which will be > > quite tricky to deal with during attach operation. How will you remove > > a particular relation from such a publication if there is a need to do > > so? > > Hmm, my understanding of how FOR ALL TABLES... features work is that > one cannot remove a particular relation from such publications? > > create schema sch; > create table sch.p (a int primary key) partition by list (a); > create table sch.p1 partition of sch.p for values in (1); > create table sch.p2 partition of sch.p for values in (2); > create table p (a int primary key) partition by list (a); > create table p1 partition of p for values in (1); > create table p2 partition of p for values in (2); > create publication puball for all tables; > create publication pubsch for all tables in schema sch; > > alter publication puball drop table p; > ERROR: publication "puball" is defined as FOR ALL TABLES > DETAIL: Tables cannot be added to or dropped from FOR ALL TABLES > publications. > > alter publication pubsch drop table sch.p; > ERROR: relation "p" is not part of the publication > > What am I missing? > Currently, in your patch, you are trying to remove a particular relation/partition during attach but how will you do that if such a relation is part of All Tables In Schema publication? > > (b) Also, how will we prohibit adding partition and its root in > > the case of "All Tables In Schema" or "All Tables" publication? If > > there is no way to do that, then that would mean we anyway need to > > deal with parent and child tables as part of the same publication and > > this new behavior will add an exception to it. > > I checked that FOR ALL TABLES publications don't allow adding a table > explicitly, but apparently IN SCHEMA one does: > > alter publication pubsch add table p2; > \dRp+ pubsch >Publication pubsch > Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root > ---++-+-+-+---+-- > amit | f | t | t | t | t | f > Tables: > "public.p2" > Tables from schemas: > "sch" > > ISTM that the ..IN SCHEMA syntax does not behave like FOR ALL TABLES > without the IN SCHEMA in this regard. Is that correct? > We do allow adding additional tables or schema to an exiting All tables in schema publication. -- With Regards, Amit Kapila.
Re: [PATCH] support tab-completion for single quote input with equal sign
At Fri, 3 Dec 2021 15:16:55 +0900, Michael Paquier wrote in > On Fri, Sep 17, 2021 at 02:45:57AM +0900, Kyotaro Horiguchi wrote: > > This test fails for the same reason, but after fixing it the result > > contains \a (BEL) in the output on my CentOS8. I'm not sure what is > > happening here.. > > The patch is still failing under the CF bot, and this last update was > two months ago. I am marking it as returned with feedback. I thought the last *WIP* patch as just a proposal of another direction after the first attempt was failed. I'll start another thread named as 'let's psql-completion share the psql command line lexer' or such, after rebasing and some polishing. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Skipping logical replication transactions on subscriber side
On Fri, Dec 3, 2021 at 12:12 PM Masahiko Sawada wrote: > > On Fri, Dec 3, 2021 at 11:53 AM Amit Kapila wrote: > > > > > But this syntax gives you flexibility, so we can also > > > start with a simple implementation. > > > > > > > Yeah, I also think so. BTW, what do you think of providing extra > > flexibility of giving other options like 'operation', 'rel' along with > > xid? I think such options could be useful for large transactions that > > operate on multiple tables as it is quite possible that only a > > particular operation from the entire transaction is the cause of > > failure. Now, on one side, we can argue that skipping the entire > > transaction is better from the consistency point of view but I think > > it is already possible that we just skip a particular update/delete > > (if the corresponding tuple doesn't exist on the subscriber). For the > > sake of simplicity, we can just allow providing xid at this stage and > > then extend it later as required but I am not very sure of that point. > > +1 > > Skipping a whole transaction by specifying xid would be a good start. > Okay, that sounds reasonable, so let's do that for now. -- With Regards, Amit Kapila.
Re: parse_subscription_options - suggested improvements
On Mon, Dec 06, 2021 at 11:28:12AM +1100, Peter Smith wrote:
> For the initialization of opts I put memset within the function to
> make it explicit that the bit-masks will work as intended without
> having to look back at calling code for the initial values. In any
> case, I think the caller declarations of SubOpts are trivial, (e.g.
> SubOpts opts = {0};) so I felt caller initializations don't need to be
> changed regardless of the memset.
It seems to me that not initializing these may cause some compilation
warnings. memset(0) at the beginning of parse_subscription_options()
is an improvement.
> My patch was meant only to remove all the redundant conditions of the
> HEAD code, so I did not rearrange any of the logic at all. Personally,
> I also think your v13 is better and easier to read, but those subtle
> behaviour differences were something I'd deliberately avoided in v12.
> However, if the committer thinks it does not matter then your v13 is
> fine by me.
Well, there is always the argument that it could be confusing as a
different combination of options generates a slightly-different error,
but the user would get warned about each one of his/her mistakes at
the end, so the result is the same.
- if (opts->enabled &&
- IsSet(supported_opts, SUBOPT_ENABLED) &&
- !IsSet(opts->specified_opts, SUBOPT_ENABLED))
+ if (opts->enabled)
I see. The last condition on the specified options in the last two
checks is removed thanks to the first two checks. As a matter of
consistency with those error strings, keeping each !IsSet() would be
cleaner. But I agree that v13 is better than that, without removing
the two initializations.
--
Michael
signature.asc
Description: PGP signature
Re: GUC flags
On Fri, Dec 03, 2021 at 10:06:47AM +0900, Michael Paquier wrote:
> On Wed, Dec 01, 2021 at 11:17:34PM -0600, Justin Pryzby wrote:
> > I find it easier to read "wait before authentication ..." than "wait ...
> > before
> > authentication".
>
> I have a hard time seeing a strong difference here. At the end, I
> have used what you suggested, adjusted the rest based on your set of
> comments, and applied the patch.
Thanks. One more item. The check_guc script currently outputs 68 false
positives - even though it includes a list of 20 exceptions. This is not
useful.
$ (cd ./src/backend/utils/misc/; ./check_guc) |wc -l
68
With the attached:
$ (cd ./src/backend/utils/misc/; ./check_guc)
config_file seems to be missing from postgresql.conf.sample
That has a defacto exception for the "include" directive, which seems
reasonable.
This requires GNU awk. I'm not sure if that's a limitation of any
significance.
--
Justin
>From 8d9130a25a59630063e644a884d354eb085faa4e Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sun, 5 Dec 2021 23:22:04 -0600
Subject: [PATCH] check_guc: fix absurd number of false positives
Avoid false positives;
Avoid various assumptions;
Avoid list of exceptions;
Simplify shell/awk/sed/grep;
This requires GNU awk for RS as a regex.
---
src/backend/utils/misc/check_guc | 69 +---
1 file changed, 10 insertions(+), 59 deletions(-)
diff --git a/src/backend/utils/misc/check_guc b/src/backend/utils/misc/check_guc
index b171ef0e4f..323ca13191 100755
--- a/src/backend/utils/misc/check_guc
+++ b/src/backend/utils/misc/check_guc
@@ -1,78 +1,29 @@
-#!/bin/sh
+#! /bin/sh
+set -e
-## currently, this script makes a lot of assumptions:
+## this script makes some assumptions about the formatting of files it parses.
## in postgresql.conf.sample:
## 1) the valid config settings may be preceded by a '#', but NOT '# '
## (we use this to skip comments)
-## 2) the valid config settings will be followed immediately by ' ='
-## (at least one space preceding the '=')
-## in guc.c:
-## 3) the options have PGC_ on the same line as the option
-## 4) the options have '{' on the same line as the option
-
-## Problems
-## 1) Don't know what to do with TRANSACTION ISOLATION LEVEL
-
-## if an option is valid but shows up in only one file (guc.c but not
-## postgresql.conf.sample), it should be listed here so that it
-## can be ignored
-INTENTIONALLY_NOT_INCLUDED="debug_deadlocks in_hot_standby \
-is_superuser lc_collate lc_ctype lc_messages lc_monetary lc_numeric lc_time \
-pre_auth_delay role seed server_encoding server_version server_version_num \
-session_authorization trace_lock_oidmin trace_lock_table trace_locks trace_lwlocks \
-trace_notify trace_userlocks transaction_isolation transaction_read_only \
-zero_damaged_pages"
### What options are listed in postgresql.conf.sample, but don't appear
### in guc.c?
-# grab everything that looks like a setting and convert it to lower case
-SETTINGS=`grep ' =' postgresql.conf.sample |
-grep -v '^# ' | # strip comments
-sed -e 's/^#//' |
-awk '{print $1}'`
-
-SETTINGS=`echo "$SETTINGS" | tr 'A-Z' 'a-z'`
+# grab everything that looks like a setting
+SETTINGS=`sed '/^#[[:alnum:]]/!d; s/^#//; s/ =.*//; /^include/d' postgresql.conf.sample`
for i in $SETTINGS ; do
- hidden=0
## it sure would be nice to replace this with an sql "not in" statement
- ## it doesn't seem to make sense to have things in .sample and not in guc.c
-# for hidethis in $INTENTIONALLY_NOT_INCLUDED ; do
-#if [ "$hidethis" = "$i" ] ; then
-# hidden=1
-#fi
-# done
- if [ "$hidden" -eq 0 ] ; then
-grep -i '"'$i'"' guc.c > /dev/null
-if [ $? -ne 0 ] ; then
- echo "$i seems to be missing from guc.c";
-fi;
- fi
+ grep -i "\"$i\"" guc.c >/dev/null ||
+echo "$i seems to be missing from guc.c";
done
### What options are listed in guc.c, but don't appear
### in postgresql.conf.sample?
# grab everything that looks like a setting and convert it to lower case
-
-SETTINGS=`grep '{.* PGC_' guc.c | awk '{print $1}' | \
- sed -e 's/{//g' -e 's/"//g' -e 's/,//'`
-
-SETTINGS=`echo "$SETTINGS" | tr 'A-Z' 'a-z'`
-
+SETTINGS=`gawk -F '[",]' 'BEGIN{RS="\n\t{\n"} /",[[:space:]]*PGC_.*.*gettext_noop/ && !/NOT_IN_SAMPLE/{print tolower($2)}' guc.c`
for i in $SETTINGS ; do
- hidden=0
- ## it sure would be nice to replace this with an sql "not in" statement
- for hidethis in $INTENTIONALLY_NOT_INCLUDED ; do
-if [ "$hidethis" = "$i" ] ; then
- hidden=1
-fi
- done
- if [ "$hidden" -eq 0 ] ; then
-grep -i '#'$i' ' postgresql.conf.sample > /dev/null
-if [ $? -ne 0 ] ; then
- echo "$i seems to be missing from postgresql.conf.sample";
-fi
- fi
+ grep "#$i " postgresql.conf.sample >/dev/null ||
+echo "$i seems to be missing from postgresql.conf.sample";
done
--
2.17.0
Re: Allow escape in application_name
At Sat, 4 Dec 2021 00:07:05 +0900, Fujii Masao wrote in > > > On 2021/11/08 22:40, kuroda.hay...@fujitsu.com wrote: > > Attached patches are the latest version. > > Thanks for updating the patch! > > + buf = makeStringInfo(); > > This is necessary only when application_name is set. So it's better to > avoid this if appname is not set. > > Currently StringInfo variable is defined in connect_pg_server() and > passed to parse_pgfdw_appname(). But there is no strong reason why the > variable needs to be defined connect_pg_server(). Right? If so how > about refactoring parse_pgfdw_appname() so that it defines > StringInfoData variable and returns the processed character string? > You can see such coding at, for example, escape_param_str() in > dblink.c. > > If this refactoring is done, probably we can get rid of "#include > "lib/stringinfo.h"" line from connection.c because connect_pg_server() > no longer needs to use StringInfo. > > + int i; > > + for (i = n - 1; i >= 0; i--) > > I'm tempted to simplify these into "for (int i = n - 1; i >= 0; i--)". > > + /* > + * Search application_name and replace it if found. > + * > + * We search paramters from the end because the later > + * one have higher priority. See also the above comment. > + */ > > How about updating these comments into the following? > > --- > Search the parameter arrays to find application_name setting, > and replace escape sequences in it with status information if found. > The arrays are searched backwards because the last value > is used if application_name is repeatedly set. > --- > > + if (strcmp(buf->data, "") != 0) > > Is this condition the same as "data[0] == '\0'"? FWIW, I agree to these. > + * parse postgres_fdw.application_name and set escaped string. > + * This function is almost same as log_line_prefix(), but > + * accepted escape sequence is different and this cannot understand > + * padding integer. > + * > + * Note that argument buf must be initialized. > > How about updating these comments into the following? > I thought that using the term "parse" was a bit confusing because what > the function actually does is to replace escape seequences in > application_name. Probably it's also better to rename the function, > e.g., to process_pgfdw_appname . It is consistent to how we have described the similar features. > - > Replace escape sequences beginning with % character in the given > application_name with status information, and return it. > - > > + if (appname == NULL) > + appname = "[unknown]"; > + if (username == NULL || *username == > '\0') > + username = "[unknown]"; > + if (dbname == NULL || *dbname == '\0') > + dbname = "[unknown]"; > > I'm still not sure why these are necessary. Could you clarify that? It probably comes from my request, just for safety for uncertain future. They are actually not needed under the current usage of the function, so *I* would not object to removing them if you are strongly feel them out of place. In that case, since NULL's leads to SEGV outright, we would even not need assertions instead. > + Same as , this is a > + printf-style string. Accepted escapes are > + bit different from , > + but padding can be used like as it. > > This description needs to be updated. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: MSVC SSL test failure
Andrew Dunstan writes: > On 12/5/21 12:50, Tom Lane wrote: >> This looks quite a bit like the sort of failure that commit >> 6051857fc was meant to forestall. I wonder whether reverting >> that commit changes the results? You might also try inserting >> a shutdown() call, as we'd decided not to do [1]. > Commenting out the closesocket() worked. Man, that's annoying. Apparently OpenSSL is doing something to screw up the shutdown sequence. According to [1], the graceful shutdown sequence will happen by default, but changing SO_LINGER or SO_DONTLINGER can get you into abortive shutdown anyway. Maybe they change one of those settings (why?) regards, tom lane [1] https://docs.microsoft.com/en-us/windows/win32/winsock/graceful-shutdown-linger-options-and-socket-closure-2
Re: Alter all tables in schema owner fix
On Fri, Dec 03, 2021 at 05:20:35PM +, Bossart, Nathan wrote: > On 12/2/21, 11:57 PM, "tanghy.f...@fujitsu.com" > wrote: > > Thanks for your patch. > > I tested it and it fixed this problem as expected. It also passed "make > > check-world". > > +1, the patch looks good to me, too. My only other suggestion would > be to move IsSchemaPublication() to pg_publication.c There is more to that, no? It seems to me that anything that opens PublicationNamespaceRelationId should be in pg_publication.c, so that would include RemovePublicationSchemaById(). If you do that, GetSchemaPublicationRelations() could be local to pg_publication.c. + tup = systable_getnext(scan); + if (HeapTupleIsValid(tup)) + result = true; This can be written as just "result = HeapTupleIsValid(tup)". Anyway, this code also means that once we drop the schema this publication won't be considered anymore as a schema publication, meaning that it also makes this code weaker to actual cache lookup failures? I find the semantics around pg_publication_namespace is bit weird because of that, and inconsistent with the existing puballtables/pg_publication_rel. -- Michael signature.asc Description: PGP signature
Re: row filtering for logical replication
On Mon, Dec 6, 2021 at 6:49 AM Euler Taveira wrote: > > On Fri, Dec 3, 2021, at 8:12 PM, Euler Taveira wrote: > > PS> I will update the commit message in the next version. I barely changed the > documentation to reflect the current behavior. I probably missed some changes > but I will fix in the next version. > > I realized that I forgot to mention a few things about the UPDATE behavior. > Regardless of 0003, we need to define which tuple will be used to evaluate the > row filter for UPDATEs. We already discussed it circa [1]. This current > version > chooses *new* tuple. Is it the best choice? But with 0003, we are using both the tuple for evaluating the row filter, so instead of fixing 0001, why we don't just merge 0003 with 0001? I mean eventually, 0003 is doing what is the agreed behavior, i.e. if just OLD is matching the filter then convert the UPDATE to DELETE OTOH if only new is matching the filter then convert the UPDATE to INSERT. Do you think that even we merge 0001 and 0003 then also there is an open issue regarding which row to select for the filter? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: preserve timestamps when installing headers
On Tue, Oct 12, 2021 at 01:22:50PM +0300, Alexander Kuzmenkov wrote: > I noticed that `make install` updates modification time for all > installed headers. This leads to recompilation of all dependent > objects, which is inconvenient for example when working on a > third-party extension. A way to solve this would be to pass > `INSTALL="install -p"` to `configure`, to make `install` preserve the > timestamp. After this, a new problem arises -- the > `src/include/Makefile` doesn't use `install` for all headers, but > instead uses `cp`. This patch adds `-p` switch to `cp` invocation in > these files, to make it preserve timestamps. Combined with the > aforementioned install flag, it allows a developer to hack on both > postgres and a third-party extension at the same time, without the > unneeded recompilation. The use of cp instead of $(INSTALL_DATA) for the installation of the headers comes from a703269, back from 2005. How do numbers compare today, 16 years later? -- Michael signature.asc Description: PGP signature
Re: row filtering for logical replication
On Sat, Dec 4, 2021 at 4:43 AM Euler Taveira wrote: > > On Thu, Dec 2, 2021, at 4:18 AM, Peter Smith wrote: > > PSA a new v44* patch set. > > We are actively developing this feature for some months and we improved this > feature a lot. This has been a good team work. It seems a good time to provide > a retrospective for this feature based on the consensus we reached until now. > > The current design has one row filter per publication-table mapping. It allows > flexible choices while using the same table for multiple replication purposes. > The WHERE clause was chosen as the syntax to declare the row filter expression > (enclosed by parentheses). > > There was a lot of discussion about which columns are allowed to use in the > row > filter expression. The consensus was that publications that publish UPDATE > and/or DELETE operations, should check if the columns in the row filter > expression is part of the replica identity. Otherwise, these DML operations > couldn't be replicated. > > We also discussed about which expression would be allowed. We couldn't allow > all kind of expressions because the way logical decoding infrastructure was > designed, some expressions could break the replication. Hence, we decided to > allow only "simple expressions". By "simple expression", we mean to restrict > (a) user-defined objects (functions, operators, types) and (b) immutable > builtin functions. > I think what you said as (b) is wrong because we want to allow builtin immutable functions. See discussion [1]. > A subscription can subscribe to multiple publications. These publication can > publish the same table. In this case, we have to combine the row filter > expression to decide if the row will be replicated or not. The consensus was > to > replicate a row if any of the row filters returns true. It means that if one > publication-table mapping does not have a row filter, the row will be > replicated. There is an optimization for this case that provides an empty > expression for this table. Hence, it bails out and replicate the row without > running the row filter code. > In addition to this, we have decided to have an exception/optimization where we need to consider publish actions while combining multiple filters as we can't combine insert/update filters. > The same logic applies to the initial table synchronization if there are > multiple row filters. Copy all rows that satisfies at least one row filter > expression. If the subscriber is a pre-15 version, data synchronization won't > use row filters if they are defined in the publisher. > > If we are dealing with partitioned tables, the publication parameter > publish_via_partition_root determines if it uses the partition row filter > (false) or the root partitioned table row filter (true). > > I used the last patch series (v44) posted by Peter Smith [1]. I did a lot of > improvements in this new version (v45). I merged 0001 (it is basically the > main > patch I wrote) and 0004 (autocomplete). As I explained in [2], I implemented a > patch (that is incorporated in the v45-0001) to fix this issue. I saw that > Peter already proposed a slightly different patch (0006). I read this patch > and > concludes that it would be better to keep the version I have. It fixes a few > things and also includes more comments. I attached another patch (v45-0002) > that includes the expression validation. It is based on 0002. I completely > overhaul it. There are additional expressions that was not supported by the > previous version (such as conditional expressions [CASE, COALESCE, NULLIF, > GREATEST, LEAST], array operators, XML operators). I probably didn't finish > the > supported node list (there are a few primitive nodes that need to be checked). > However, the current "simple expression" routine seems promising. I plan to > integrate v45-0002 in the next patch version. I attached it here for > comparison > purposes only. > > My next step is to review 0003. As I said before it would like to treat it as > a > separate feature. > I don't think that would be right decision as we already had discussed that in detail and reach to the current conclusion based on which Ajin's 0003 patch is. > I know that it is useful for data consistency but this patch > is already too complex. > True, but that is the main reason the review and development are being done as separate sub-features. I suggest still keeping the similar separation till some of the reviews of each of the patches are done, otherwise, we need to rethink how to divide for easier review. We need to retain the 0005 patch because that handles many problems without which the main patch is incomplete and buggy w.r.t replica identity. [1] - https://www.postgresql.org/message-id/CAA4eK1%2BXoD49bz5-2TtiD0ugq4PHSRX2D1sLPR_X4LNtdMc4OQ%40mail.gmail.com -- With Regards, Amit Kapila.
Re: preserve timestamps when installing headers
Michael Paquier writes: > On Tue, Oct 12, 2021 at 01:22:50PM +0300, Alexander Kuzmenkov wrote: >> This patch adds `-p` switch to `cp` invocation in >> these files, to make it preserve timestamps. > The use of cp instead of $(INSTALL_DATA) for the installation of the > headers comes from a703269, back from 2005. How do numbers compare > today, 16 years later? According to a nearby copy of POSIX, "cp -p" does a lot more than preserve timestamps. It also specifies preserving file ownership, which seems absolutely catastrophic for the standard use-case of "build as some ordinary user, then install as root". TBH, I am not convinced that the complained-of case is enough of a problem to justify any change in our build rules, even if there weren't any semantic issues. If you are worried about build times, you should be using ccache, and IME builds using ccache are not terribly impacted by file timestamp changes. regards, tom lane
Re: row filtering for logical replication
On Mon, Dec 6, 2021 at 12:06 PM Dilip Kumar wrote: > > On Mon, Dec 6, 2021 at 6:49 AM Euler Taveira wrote: > > > > On Fri, Dec 3, 2021, at 8:12 PM, Euler Taveira wrote: > > > > PS> I will update the commit message in the next version. I barely changed > > the > > documentation to reflect the current behavior. I probably missed some > > changes > > but I will fix in the next version. > > > > I realized that I forgot to mention a few things about the UPDATE behavior. > > Regardless of 0003, we need to define which tuple will be used to evaluate > > the > > row filter for UPDATEs. We already discussed it circa [1]. This current > > version > > chooses *new* tuple. Is it the best choice? > > But with 0003, we are using both the tuple for evaluating the row > filter, so instead of fixing 0001, why we don't just merge 0003 with > 0001? > I agree that would be better than coming up with an entirely new approach especially when the current approach is discussed and agreed upon. > I mean eventually, 0003 is doing what is the agreed behavior, > i.e. if just OLD is matching the filter then convert the UPDATE to > DELETE OTOH if only new is matching the filter then convert the UPDATE > to INSERT. +1. > Do you think that even we merge 0001 and 0003 then also > there is an open issue regarding which row to select for the filter? > I think eventually we should merge 0001 and 0003 to avoid any sort of data consistency but it is better to keep them separate for the purpose of a review at this stage. If I am not wrong that still needs bug-fix we are discussing it as part of CF entry [1], right? If so, isn't it better to review that bug-fix patch and the 0003 patch being discussed here [2] to avoid missing any already reported issues in this thread? [1] - https://commitfest.postgresql.org/36/3162/ [2] - https://www.postgresql.org/message-id/CAHut%2BPtJnnM8MYQDf7xCyFAp13U_0Ya2dv-UQeFD%3DghixFLZiw%40mail.gmail.com -- With Regards, Amit Kapila.
RE: pg_get_publication_tables() output duplicate relid
On Thursday, December 2, 2021 9:48 PM Amit Langote wrote: > On Thu, Dec 2, 2021 at 2:27 PM Amit Kapila wrote: > > On Thu, Dec 2, 2021 at 8:42 AM Amit Langote > wrote: > > > Reading Alvaro's email at the top again gave me a pause to > > > reconsider what I had said in reply. It might indeed have been nice > > > if the publication DDL itself had prevented the cases where a > > > partition and its ancestor are added to a publication, though as > > > Hou-san mentioned, partition ATTACHes make that a bit tricky to > > > enforce at all times, though of course not impossible. Maybe it's > > > okay to just de-duplicate pg_publication_tables output as the patch > > > does, even though it may appear to be a band-aid solution if we one > > > considers Alvaro's point about consistency. > > > > True, I think even if we consider that idea it will be a much bigger > > change, and also as it will be a behavioral change so we might want to > > keep it just for HEAD and some of these fixes need to be backpatched. > > Having said that, if you or someone want to pursue that idea and come > > up with a better solution than what we have currently it is certainly > > welcome. > > Okay, I did write a PoC patch this morning after sending out my earlier > email. I > polished it a bit, which is attached. After thinking more on this. I find there might be some usage about adding both child and parent to the publication. For the future feature publication row filter(and maybe column filter), It could be useful for user to adding child and parent with different filter expression. If pubviaroot=true, user can expect the parent's filter take effect, If pubviaroot=false, they can expect the child's filter take effect. If we disallow adding both child and parent to publication, it could be harder for these features to implement. So, personally, I am not sure disallow adding both child and parent to the publication is a good idea. Best regards, Hou zj
RE: Optionally automatically disable logical replication subscriptions on error
On Monday, December 6, 2021 1:38 PM Mark Dilger wrote: > > On Dec 1, 2021, at 8:48 PM, Amit Kapila wrote: > > > > The patch disables the subscription for non-transient errors. I am not > > sure if we can easily make the call to decide whether any particular > > error is transient or not. For example, DISK_FULL or OUT_OF_MEMORY > > might not rectify itself. Why not just allow to disable the > > subscription on any error? And then let the user check the error > > either in view or logs and decide whether it would like to enable the > > subscription or do something before it (like making space in disk, or > > fixing the network). > > The original idea of the patch, back when I first wrote and proposed it, was > to > remove the *absurdity* of retrying a transaction which, in the absence of > human intervention, was guaranteed to simply fail again ad infinitum. > Retrying in the face of resource errors is not *absurd* even though it might > fail > again ad infinitum. The reason is that there is at least a chance that the > situation will clear up without human intervention. In my humble opinion, I felt the original purpose of the patch was to partially remedy the situation that during the failure of apply, the apply process keeps going into the infinite error loop. I'd say that in this sense, if we include such resource errors, we fail to achieve the purpose in some cases, because of some left possibilities of infinite loop. Disabling the subscription with even one any error excludes this irregular possibility, since there's no room to continue the infinite loop. Best Regards, Takamichi Osumi
Re: GUC flags
On Sun, Dec 05, 2021 at 11:38:05PM -0600, Justin Pryzby wrote: > Thanks. One more item. The check_guc script currently outputs 68 false > positives - even though it includes a list of 20 exceptions. This is not > useful. Indeed. Hmm. This script does a couple of things: 1) Check the format of the options defined in the various lists of guc.c, which is something people format well, and pgindent also does a part of this job. 2) Check that options in the hardcoded list of GUCs in INTENTIONALLY_NOT_INCLUDED are not included in postgresql.conf.sample 3) Check that nothing considered as a parameter in postgresql.conf.sample is listed in guc.c. Your patch removes 1) and 2), but keeps 3) to check for dead parameter references in postgresql.conf.sample. Is check_guc actually run on a periodic basis by somebody? Based on the amount of false positives that has accumulated over the years, and what `git grep` can already do for 3), it seems to me that we have more arguments in favor of just removing it entirely. -- Michael signature.asc Description: PGP signature
Re: pg_replslotdata - a tool for displaying replication slot information
On Thu, Dec 02, 2021 at 08:32:08AM +0530, Bharath Rupireddy wrote: > On Thu, Dec 2, 2021 at 4:22 AM Andres Freund wrote: >>> I don't have any other compelling use- cases at the moment, but I will say >>> that it is typically nice from an administrative standpoint to be able to >>> inspect things like this without logging into a running server. >> >> Weighed against the cost of maintaining (including documentation) a separate >> tool this doesn't seem sufficient reason. > > IMHO, this shouldn't be a reason to not get something useful (useful > IMO and few others in this thread) into the core. The maintenance of > the tools generally is low compared to the core server features once > they get reviewed and committed. Well, a bit less maintenance is always better than more maintenance. An extra cost that you may be missing is related to the translation of the documentation, as well as the translation of any new strings this would require. FWIW, I don't directly see a use for this tool that could not be solved with an online server. -- Michael signature.asc Description: PGP signature
Re: pg_get_publication_tables() output duplicate relid
On Mon, Dec 6, 2021 at 3:55 PM houzj.f...@fujitsu.com wrote: > On Thursday, December 2, 2021 9:48 PM Amit Langote > wrote: > > On Thu, Dec 2, 2021 at 2:27 PM Amit Kapila wrote: > > > On Thu, Dec 2, 2021 at 8:42 AM Amit Langote > > wrote: > > > > Reading Alvaro's email at the top again gave me a pause to > > > > reconsider what I had said in reply. It might indeed have been nice > > > > if the publication DDL itself had prevented the cases where a > > > > partition and its ancestor are added to a publication, though as > > > > Hou-san mentioned, partition ATTACHes make that a bit tricky to > > > > enforce at all times, though of course not impossible. Maybe it's > > > > okay to just de-duplicate pg_publication_tables output as the patch > > > > does, even though it may appear to be a band-aid solution if we one > > > > considers Alvaro's point about consistency. > > > > > > True, I think even if we consider that idea it will be a much bigger > > > change, and also as it will be a behavioral change so we might want to > > > keep it just for HEAD and some of these fixes need to be backpatched. > > > Having said that, if you or someone want to pursue that idea and come > > > up with a better solution than what we have currently it is certainly > > > welcome. > > > > Okay, I did write a PoC patch this morning after sending out my earlier > > email. I > > polished it a bit, which is attached. > > After thinking more on this. I find there might be some usage about adding > both > child and parent to the publication. > > For the future feature publication row filter(and maybe column filter), It > could be useful for user to adding child and parent with different filter > expression. If pubviaroot=true, user can expect the parent's filter take > effect, If pubviaroot=false, they can expect the child's filter take effect. > > If we disallow adding both child and parent to publication, it could be harder > for these features to implement. It's possible that one may want such a feature, and yes, outright preventing adding both the parent and a partition to a publication would perhaps make it harder. Mind you though that any such feature would need to fix the current implementation anyway to make the publication-behavior-related catalog entries for child tables, which we currently don't, unless children are added explicitly. That said, we should also be careful in implementing new features that offer a finer per-partition granularity, because the fact that we do currently offer that for many of the existing old features such as constraints, indexes, etc. limits the scalability of our architecture. That point may be off-topic for the thread, but something to consider, or maybe I need to take a look at the other patch to see if my concerns are addressable / have been addressed. -- Amit Langote EDB: http://www.enterprisedb.com
Make pg_waldump report replication origin ID, LSN, and timestamp.
Hi all,
I realized that pg_waldump doesn't show replication origin ID, LSN,
and timestamp of PREPARE TRANSACTION record. Commit 7b8a899bdeb
improved pg_waldump two years ago so that it reports the detail of
information of PREPARE TRANSACTION but ISTM that it overlooked showing
replication origin information. As far as I can see in the discussion
thread[1], there was no discussion on that. These are helpful when
diagnosing 2PC related issues on the subscriber side.
I've attached a patch to add replication origin information to
xact_desc_prepare().
Regards,
[1]
https://www.postgresql.org/message-id/CAHGQGwEvhASad4JJnCv%3D0dW2TJypZgW_Vpb-oZik2a3utCqcrA%40mail.gmail.com
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
From b37d5a8cd5b84eeb850bd7ddeb903024652f20d2 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada
Date: Mon, 6 Dec 2021 14:32:57 +0900
Subject: [PATCH] Make pg_waldump report replication origin ID, LSN, and
timestamp.
Commit 7b8a899bdeb made pg_waldump report the detail of information
about PREPARE TRANSACTION record, like global transaction
identifier. However, replication origin LSN and timestamp were not
reporeted. These are helpful when diagnosing 2PC-related troubles on
the subsciber side.
This commit makes xact_desc_prepare() report replication origin ID,
LSN, and timestamp.
---
src/backend/access/rmgrdesc/xactdesc.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/rmgrdesc/xactdesc.c b/src/backend/access/rmgrdesc/xactdesc.c
index 4b0d10f073..ea3649d822 100644
--- a/src/backend/access/rmgrdesc/xactdesc.c
+++ b/src/backend/access/rmgrdesc/xactdesc.c
@@ -16,6 +16,7 @@
#include "access/transam.h"
#include "access/xact.h"
+#include "replication/origin.h"
#include "storage/sinval.h"
#include "storage/standbydefs.h"
#include "utils/timestamp.h"
@@ -329,7 +330,7 @@ xact_desc_abort(StringInfo buf, uint8 info, xl_xact_abort *xlrec)
}
static void
-xact_desc_prepare(StringInfo buf, uint8 info, xl_xact_prepare *xlrec)
+xact_desc_prepare(StringInfo buf, uint8 info, xl_xact_prepare *xlrec, RepOriginId origin_id)
{
xl_xact_parsed_prepare parsed;
@@ -345,6 +346,12 @@ xact_desc_prepare(StringInfo buf, uint8 info, xl_xact_prepare *xlrec)
standby_desc_invalidations(buf, parsed.nmsgs, parsed.msgs, parsed.dbId,
parsed.tsId, xlrec->initfileinval);
+
+ if (origin_id != InvalidRepOriginId)
+ appendStringInfo(buf, "; origin: node %u, lsn %X/%X, at %s",
+ origin_id,
+ LSN_FORMAT_ARGS(parsed.origin_lsn),
+ timestamptz_to_str(parsed.origin_timestamp));
}
static void
@@ -381,7 +388,8 @@ xact_desc(StringInfo buf, XLogReaderState *record)
{
xl_xact_prepare *xlrec = (xl_xact_prepare *) rec;
- xact_desc_prepare(buf, XLogRecGetInfo(record), xlrec);
+ xact_desc_prepare(buf, XLogRecGetInfo(record), xlrec,
+ XLogRecGetOrigin(record));
}
else if (info == XLOG_XACT_ASSIGNMENT)
{
--
2.24.3 (Apple Git-128)
