Re: query_id, pg_stat_activity, extended query protocol
On Tue, Apr 23, 2024 at 11:42:41AM +0700, Andrei Lepikhov wrote:
> On 23/4/2024 11:16, Imseih (AWS), Sami wrote:
>> + pgstat_report_query_id(linitial_node(Query,
>> psrc->query_list)->queryId, true);
>> set_ps_display("BIND");
>> @@ -2146,6 +2147,7 @@ exec_execute_message(const char *portal_name, long
>> max_rows)
>> debug_query_string = sourceText;
>> pgstat_report_activity(STATE_RUNNING, sourceText);
>> + pgstat_report_query_id(portal->queryDesc->plannedstmt->queryId,
>> true);
>> cmdtagname = GetCommandTagNameAndLen(portal->commandTag,
>> );
>
> In exec_bind_message, how can you be sure that queryId exists in query_list
> before the call of GetCachedPlan(), which will validate and lock the plan?
> What if some OIDs were altered in the middle?
I am also a bit surprised with the choice of using the first Query
available in the list for the ID, FWIW.
Did you consider using \bind to show how this behaves in a regression
test?
--
Michael
signature.asc
Description: PGP signature
Re: Direct SSL connection with ALPN and HBA rules
On Mon, Apr 22, 2024 at 10:47:51AM +0300, Heikki Linnakangas wrote: > On 22/04/2024 10:19, Michael Paquier wrote: >> As a whole, I can get behind a unique GUC that forces the use of >> direct. Or, we could extend the existing "ssl" GUC with a new >> "direct" value to accept only direct connections and restrict the >> original protocol (and a new "postgres" for the pre-16 protocol, >> rejecting direct?), while "on" is able to accept both. > > I'd be OK with that, although I still don't really see the point of forcing > this from the server side. We could also add this later. I'd be OK with doing something only in v18, if need be. Jacob, what do you think? >> Hmm. Splitting the logic checking HBA entries (around check_hba) so >> as we'd check for a portion of its contents depending on what the >> server has received or not from the client would not be that >> complicated. I'd question whether it makes sense to mix this >> information within the same configuration files as the ones holding >> the current HBA rules. If the same rules are used for the >> pre-startup-packet phase and the post-startup-packet phase, we'd want >> new keywords for these HBA rules, something different than the >> existing sslmode and no sslmode? > > Sounds complicated, and I don't really see the use case for controlling the > direct SSL support in such a fine-grained fashion. > > It would be nice if we could reject non-SSL connections before the client > sends the startup packet, but that's not possible because in a plaintext > connection, that's the first packet that the client sends. The reverse would > be possible: reject SSLRequest or direct SSL connection immediately, if HBA > doesn't allow non-SSL connections from that IP address. But that's not very > interesting. I'm not completely sure, actually. We have the APIs to do that in simple ways with existing keywords and new options. And there is some merit being able to have more complex connection policies. If both of you object to that, I won't insist. > HBA-based control would certainly be v18 material. Surely. -- Michael signature.asc Description: PGP signature
Is it acceptable making COPY format extendable?
Hi, I'm proposing a patch that making COPY format extendable: https://www.postgresql.org/message-id/20231204.153548.2126325458835528809.kou%40clear-code.com https://commitfest.postgresql.org/48/4681/ It's based on the discussion at: https://www.postgresql.org/message-id/flat/3741749.1655952...@sss.pgh.pa.us#2bb7af4a3d2c7669f9a49808d777a20d > > > IIUC, you want extensibility in FORMAT argument to COPY command > > > https://www.postgresql.org/docs/current/sql-copy.html. Where the > > > format is pluggable. That seems useful. > > > Agreed. > > Ditto. But my proposal stalled. It it acceptable the feature request that making COPY format extendable? If it's not acceptable, what are blockers? Performance? Thanks, -- kou
Re: Support a wildcard in backtrace_functions
On Mon, Apr 22, 2024 at 09:25:15AM -0400, Robert Haas wrote: > That's long been my feeling about this. So, if we revert this for now, > what we ought to do is put it back right ASAP after feature freeze and > then clean all that up. In the 85 backtraces I can find in the tests, we have a mixed bag of: - Code paths that use the internal errcode, but should not. - Code paths that use the internal errcode, and are OK with that in the scope of the tests. - Code paths that use the internal errcode, though the coding assumptions behind their use feels fuzzy to me, like amcheck for some SQL tests or satisfies_hash_partition() in one SQL test. As cleaning up that is a separate topic, I have begun a new thread and with a full analysis of everything I've spotted. See here: https://www.postgresql.org/message-id/zic_gngos5smx...@paquier.xyz The first class of issues refers to real problems, and should be assigned errcodes. Having a way to switch the backtraces off can have some benefits in the second cases. However, even if we silence them, it would also mean to miss backtraces that could be legit. The third cases would require a different analysis, behind the designs of the code paths able to trigger the internal codes. At this stage, my opinion would tend in favor of a revert of the GUC. The second class of cases is useful to stress many unexpected cases, and I don't expect this number to go down over time, but increase with more advanced tests added into core (I/O failures with partial writes for availability, etc). -- Michael signature.asc Description: PGP signature
Re: Avoid orphaned objects dependencies, take 3
Hi, On Mon, Apr 22, 2024 at 03:00:00PM +0300, Alexander Lakhin wrote: > 22.04.2024 13:52, Bertrand Drouvot wrote: > > > > That's weird, I just launched it several times with the patch applied and > > I'm not > > able to see the seg fault (while I can see it constently failing on the > > master > > branch). > > > > Are you 100% sure you tested it against a binary with the patch applied? > > > > Yes, at least I can't see what I'm doing wrong. Please try my > self-contained script attached. Thanks for sharing your script! Yeah your script ensures the patch is applied before the repro is executed. I do confirm that I can also see the issue with the patch applied (but I had to launch multiple attempts, while on master one attempt is enough). I'll have a look. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Internal error codes triggered by tests
Hi all,
While analyzing the use of internal error codes in the code base, I've
some problems, and that's a mixed bag of:
- Incorrect uses, for errors that can be triggered by users with
vallid cases.
- Expected error cases, wanted by the tests like corruption cases or
just to keep some code simpler.
Here is a summary of the ones that should be fixed with proper
errcodes:
1) be-secure-openssl.c is forgetting an error codes for code comparing
the ssl_{min,max}_protocol_version range, which should be a
ERRCODE_CONFIG_FILE_ERROR.
2) 010_pg_basebackup.pl includes a test for an unmatching system ID at
its bottom, that triggers an internal error as an effect of
manifest_report_error().
3) basebackup.c, with a too long symlink or tar name, where
ERRCODE_PROGRAM_LIMIT_EXCEEDED should make sense.
4) pg_walinspect, for an invalid LSN. That would be
ERRCODE_INVALID_PARAMETER_VALUE.
5) Some paths of pg_ucol_open() are failing internally, still these
refer to parameters that can be set, so I've attached
ERRCODE_INVALID_PARAMETER_VALUE to them.
6) PerformWalRecovery(), where recovery ends before target is reached,
for a ERRCODE_CONFIG_FILE_ERROR.
7) pg_replication_slot_advance(), missing for the target LSN a
ERRCODE_INVALID_PARAMETER_VALUE.
8) Isolation test alter-table-4/s2, able to trigger a "attribute "a"
of relation "c1" does not match parent's type". Shouldn't that be a
ERRCODE_INVALID_COLUMN_DEFINITION?
Then there is a series of issues triggered by the main regression test
suite, applying three times (pg_upgrade, make check and
027_stream_regress.pl):
1) MergeConstraintsIntoExisting() under a case of relhassubclass, for
ERRCODE_INVALID_OBJECT_DEFINITION.
2) Trigger rename on a partition, see renametrig(), for
ERRCODE_FEATURE_NOT_SUPPORTED.
3) "could not find suitable unique index on materialized view", with a
plain elog() in matview.c, for ERRCODE_FEATURE_NOT_SUPPORTED
4) "role \"blah\" cannot have explicit members", for
ERRCODE_FEATURE_NOT_SUPPORTED.
5) Similar to previous one, AddRoleMems() with "role \"blah\" cannot
be a member of any role"
6) icu_validate_locale(), icu_language_tag() and make_icu_collator()
for invalid parameter inputs.
7) ATExecAlterConstraint()
There are a few fancy cases where we expect an internal error per the
state of the tests:
1) amcheck
1-1) bt_index_check_internal() in amcheck, where the code has the idea
to open an OID given in input, trigerring an elog(). That does not
strike me as a good idea, though that's perhaps acceptable. The error
is an "could not open relation with OID".
1-2) 003_check.pl has 12 cases with backtraces expected in the outcome
as these check corruption cases.
2) pg_visibility does the same thing, for two tests trigerring a
"could not open relation with OID".
3) plpython with 6 cases which are internal, not sure what to do about
these.
4) test_resowner has two cases triggered by SQL functions, which are
expected to be internal.
5) test_tidstore, with "tuple offset out of range" triggered by a SQL
call.
6) injection_points, that are aimed for tests, has six backtraces.
7) currtid_internal().. Perhaps we should try to rip out this stuff,
which is specific to ODBC. There are a lot of backtraces here.
8) satisfies_hash_partition() in test hash_part, generating a
backtrace for an InvalidOid in the main regression test suite.
All these cases are able to trigger backtraces, and while of them are
OK to keep as they are, the cases of the first and second lists ought
to be fixed, and attached is a patch do close the gap. This reduces
the number of internal errors generated by the tests from 85 to 35,
with injection points enabled.
Note that I've kept the currtid() errcodes in it, though I don't think
much of them. The rest looks sensible enough to address.
Thoughts?
--
Michael
From 8386cf2afd1496bea159100bdccb6afd93c6e1c8 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 23 Apr 2024 13:53:28 +0900
Subject: [PATCH] Assign some errcodes where missing
---
src/backend/access/transam/xlogrecovery.c | 3 ++-
src/backend/backup/basebackup.c | 6 --
src/backend/commands/matview.c| 4 +++-
src/backend/commands/tablecmds.c | 4 +++-
src/backend/commands/trigger.c| 1 +
src/backend/commands/user.c | 2 ++
src/backend/libpq/be-secure-openssl.c | 3 ++-
src/backend/optimizer/util/appendinfo.c | 12
src/backend/replication/slotfuncs.c | 3 ++-
src/backend/utils/adt/pg_locale.c | 21 +---
src/backend/utils/adt/tid.c | 24 ---
contrib/pg_walinspect/pg_walinspect.c | 3 ++-
12 files changed, 60 insertions(+), 26 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index 29c5bec084..a6543b539e 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -1898,7 +1898,8 @@ PerformWalRecovery(void)
Re: query_id, pg_stat_activity, extended query protocol
On 23/4/2024 11:16, Imseih (AWS), Sami wrote:
FWIW, I'd like to think that we could improve the situation, requiring
a mix of calling pgstat_report_query_id() while feeding on some query
IDs retrieved from CachedPlanSource->query_list. I have not in
details looked at how much could be achieved, TBH.
I was dealing with this today and found this thread. I spent some time
looking at possible solutions.
In the flow of extended query protocol, the exec_parse_message
reports the queryId, but subsequent calls to exec_bind_message
and exec_execute_message reset the queryId when calling
pgstat_report_activity(STATE_RUNNING,..) as you can see below.
/*
* If a new query is started, we reset the query identifier as it'll only
* be known after parse analysis, to avoid reporting last query's
* identifier.
*/
if (state == STATE_RUNNING)
beentry->st_query_id = UINT64CONST(0);
So, I think the simple answer is something like the below.
Inside exec_bind_message and exec_execute_message,
the query_id should be reported after pg_report_activity.
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 76f48b13d2..7ec2df91d5 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1678,6 +1678,7 @@ exec_bind_message(StringInfo input_message)
debug_query_string = psrc->query_string;
pgstat_report_activity(STATE_RUNNING, psrc->query_string);
+ pgstat_report_query_id(linitial_node(Query, psrc->query_list)->queryId,
true);
set_ps_display("BIND");
@@ -2146,6 +2147,7 @@ exec_execute_message(const char *portal_name, long max_rows)
debug_query_string = sourceText;
pgstat_report_activity(STATE_RUNNING, sourceText);
+ pgstat_report_query_id(portal->queryDesc->plannedstmt->queryId, true);
cmdtagname = GetCommandTagNameAndLen(portal->commandTag, );
thoughts?
In exec_bind_message, how can you be sure that queryId exists in
query_list before the call of GetCachedPlan(), which will validate and
lock the plan? What if some OIDs were altered in the middle?
--
regards, Andrei Lepikhov
Re: query_id, pg_stat_activity, extended query protocol
> FWIW, I'd like to think that we could improve the situation, requiring
> a mix of calling pgstat_report_query_id() while feeding on some query
> IDs retrieved from CachedPlanSource->query_list. I have not in
> details looked at how much could be achieved, TBH.
I was dealing with this today and found this thread. I spent some time
looking at possible solutions.
In the flow of extended query protocol, the exec_parse_message
reports the queryId, but subsequent calls to exec_bind_message
and exec_execute_message reset the queryId when calling
pgstat_report_activity(STATE_RUNNING,..) as you can see below.
/*
* If a new query is started, we reset the query identifier as it'll only
* be known after parse analysis, to avoid reporting last query's
* identifier.
*/
if (state == STATE_RUNNING)
beentry->st_query_id = UINT64CONST(0);
So, I think the simple answer is something like the below.
Inside exec_bind_message and exec_execute_message,
the query_id should be reported after pg_report_activity.
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 76f48b13d2..7ec2df91d5 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1678,6 +1678,7 @@ exec_bind_message(StringInfo input_message)
debug_query_string = psrc->query_string;
pgstat_report_activity(STATE_RUNNING, psrc->query_string);
+ pgstat_report_query_id(linitial_node(Query, psrc->query_list)->queryId,
true);
set_ps_display("BIND");
@@ -2146,6 +2147,7 @@ exec_execute_message(const char *portal_name, long
max_rows)
debug_query_string = sourceText;
pgstat_report_activity(STATE_RUNNING, sourceText);
+ pgstat_report_query_id(portal->queryDesc->plannedstmt->queryId, true);
cmdtagname = GetCommandTagNameAndLen(portal->commandTag, );
thoughts?
Regards,
Sami Imseih
Amazon Web Services (AWS)
Re: clamp_row_est avoid infinite
jian he writes: > if (nrows > MAXIMUM_ROWCOUNT || isnan(nrows)) > nrows = MAXIMUM_ROWCOUNT; > else if (nrows <= 1.0) > nrows = 1.0; > else > nrows = rint(nrows); > The comments say `Avoid infinite and NaN` > but actually we only avoid NaN. Really? The IEEE float arithmetic standard says that Inf is greater than any finite value, and in particular it'd be greater than MAXIMUM_ROWCOUNT. regards, tom lane
Re: Statistics Import and Export
Jeff Davis writes: > On Mon, 2024-04-22 at 16:19 -0400, Tom Lane wrote: >> Loading data without stats, and hoping >> that auto-analyze will catch up sooner not later, is exactly the >> current behavior that we're doing all this work to get out of. > That's the disconnect, I think. For me, the main reason I'm excited > about this work is as a way to solve the bad-plans-after-upgrade > problem and to repro planner issues outside of production. Avoiding the > need to ANALYZE at the end of a data load is also a nice convenience, > but not a primary driver (for me). Oh, I don't doubt that there are use-cases for dumping stats without data. I'm just dubious about the reverse. I think data+stats should be the default, even if only because pg_dump's default has always been to dump everything. Then there should be a way to get stats only, and maybe a way to get data only. Maybe this does argue for a four-section definition, despite the ensuing churn in the pg_dump API. > Should we just itemize some common use cases for pg_dump, and then > choose the defaults that are least likely to cause surprise? Per above, I don't find any difficulty in deciding what should be the default. What I think we need to consider is what the pg_dump and pg_restore switch sets should be. There's certainly a few different ways we could present that; maybe we should sketch out the details for a couple of ways. regards, tom lane
Re: promotion related handling in pg_sync_replication_slots()
On Mon, Apr 22, 2024 at 7:04 PM Masahiko Sawada wrote:
>
> On Mon, Apr 22, 2024 at 9:02 PM shveta malik wrote:
> >
> > Thanks for the patch, the changes look good Amit. Please find the merged
> > patch.
> >
>
> I've reviewed the patch and have some comments:
>
> ---
> /*
> -* Early initialization.
> +* Register slotsync_worker_onexit() before we register
> +* ReplicationSlotShmemExit() in BaseInit(), to ensure that during the
> +* exit of the slot sync worker, ReplicationSlotShmemExit() is called
> +* first, followed by slotsync_worker_onexit(). The startup process during
> +* promotion invokes ShutDownSlotSync() which waits for slot sync to
> +* finish and it does that by checking the 'syncing' flag. Thus worker
> +* must be done with the slots' release and cleanup before it marks itself
> +* as finished syncing.
> */
>
> I'm slightly worried that we register the slotsync_worker_onexit()
> callback before BaseInit(), because it could be a blocker when we want
> to add more work in the callback, for example sending the stats.
>
The other possibility is that we do slot release/clean up in the
slotsync_worker_onexit() call itself and then we can do it after
BaseInit(). Do you have any other/better idea for this?
> ---
> synchronize_slots(wrconn);
> +
> + /* Cleanup the temporary slots */
> + ReplicationSlotCleanup();
> +
> + /* We are done with sync, so reset sync flag */
> + reset_syncing_flag();
>
> I think it ends up removing other temp slots that are created by the
> same backend process using
> pg_create_{physical,logical_replication_slots() function, which could
> be a large side effect of this function for users.
>
True, I think here we should either remove only temporary and synced
marked slots. The other possibility is to create slots as RS_EPHEMERAL
initially when called from the SQL function but that doesn't sound
like a neat approach.
>
Also, if users want
> to have a process periodically calling pg_sync_replication_slots()
> instead of the slotsync worker, it doesn't support a case where we
> create a temp not-ready slot and turn it into a persistent slot if
> it's ready for sync.
>
True, but eventually the API should be able to directly create the
persistent slots and anyway this can happen only for the first time
(till the slots are created and marked persistent) and one who wants
to use this function periodically should be able to see regular syncs.
OTOH, leaving temp slots created via this API could remain as-is after
promotion and we need to document for users to remove such slots. Now,
we can do that if we want but I think it is better to clean up such
slots rather than putting the onus on users to remove them after
promotion.
--
With Regards,
Amit Kapila.
clamp_row_est avoid infinite
hi.
/*
* clamp_row_est
* Force a row-count estimate to a sane value.
*/
double
clamp_row_est(double nrows)
{
/*
* Avoid infinite and NaN row estimates. Costs derived from such values
* are going to be useless. Also force the estimate to be at least one
* row, to make explain output look better and to avoid possible
* divide-by-zero when interpolating costs. Make it an integer, too.
*/
if (nrows > MAXIMUM_ROWCOUNT || isnan(nrows))
nrows = MAXIMUM_ROWCOUNT;
else if (nrows <= 1.0)
nrows = 1.0;
else
nrows = rint(nrows);
return nrows;
}
The comments say `Avoid infinite and NaN`
but actually we only avoid NaN.
Do we need to add isinf(nrows)?
Re: logicalrep_worker_launch -- counting/checking the worker limits
On Sun, Mar 31, 2024 at 8:12 PM Andrey M. Borodin wrote: > > > > > On 15 Aug 2023, at 07:38, Peter Smith wrote: > > > > A rebase was needed due to a recent push [1]. > > > > PSA v3. > > > > On 14 Jan 2024, at 10:43, vignesh C wrote: > > > > I have changed the status of the patch to "Waiting on Author" as > > Amit's queries at [1] have not been verified and concluded. Please > > feel free to address them and change the status back again. > > Hi Peter! > > Are you still interested in this thread? If so - please post an answer to > Amit's question. > If you are not interested - please Withdraw a CF entry [0]. > > Thanks! Yeah, sorry for the long period of inactivity on this thread. Although I still have some interest in it, I don't know when I'll get back to it again so meantime I've withdrawn this from the CF as requested. Kind regards, Peter Smith Fujitsu Australia
Re: Statistics Import and Export
On Mon, 2024-04-22 at 16:19 -0400, Tom Lane wrote: > Loading data without stats, and hoping > that auto-analyze will catch up sooner not later, is exactly the > current behavior that we're doing all this work to get out of. That's the disconnect, I think. For me, the main reason I'm excited about this work is as a way to solve the bad-plans-after-upgrade problem and to repro planner issues outside of production. Avoiding the need to ANALYZE at the end of a data load is also a nice convenience, but not a primary driver (for me). Should we just itemize some common use cases for pg_dump, and then choose the defaults that are least likely to cause surprise? As for the section, I'm not sure what to do about that. Based on this thread it seems that SECTION_NONE (or a SECTION_STATS?) is easiest to implement, but I don't understand the long-term consequences of that choice. Regards, Jeff Davis
Re: fix tablespace handling in pg_combinebackup
Hi, On 2024-04-22 18:10:17 -0400, Robert Haas wrote: > cfbot is giving me a bunch of green check marks, so I plan to commit > this version, barring objections. Makes sense. > I shall leave redesign of the symlink mess as a matter for others to > ponder; I'm not in love with what we have, but I think it will be > tricky to do better, and I don't think I want to spend time on it, at > least not now. Oh, yea, that's clearly a much bigger and separate project... Greetings, Andres Freund
Re: Cleanup: remove unused fields from nodes
On Mon, Apr 22, 2024 at 06:46:27PM +0200, Matthias van de Meent wrote: > On Mon, 22 Apr 2024 at 17:41, Tom Lane wrote: >> Matthias van de Meent writes: >>> 0002/0004 remove fields in ExecRowMark which were added for FDWs to >>> use, but there are no FDWs which use this: I could only find two FDWs >>> who implement RefetchForeignRow, one being BlackHoleFDW, and the other >>> a no-op implementation in kafka_fdw [0]. We also don't seem to have >>> any testing on this feature. >> >> I'm kind of down on removing either of these. ermExtra is explicitly >> intended for extensions to use, and just because we haven't found any >> users doesn't mean there aren't any, or might not be next week. > > That's a good point, and also why I wasn't 100% sure removing it was a > good idea. I'm not quite sure why this would be used (rather than the > internal state of the FDW, or no state at all), but haven't looked > very deep into it either, so I'm quite fine with not channging that. Custom nodes are one extra possibility? I'd leave ermActive and ermExtra be. >> I think it would be a good idea to push 0003 for v17, just so nobody >> grows an unnecessary dependency on that field. 0001 and 0005 could >> be left for v18, but on the other hand they're so trivial that it >> could also be sensible to just push them to get them out of the way. > > Beta 1 scheduled to be released for quite some time, so I don't think > there are any problems with fixing these kinds of minor issues in the > provisional ABI for v17. Tweaking the APIs should be OK until GA, as long as we agree that the current interfaces can be improved. 0003 is new in v17, so let's apply it now. I don't see much a strong argument in waiting for the removal of 0001 and 0005, either, to keep the interfaces cleaner moving on. However, this is not a regression and these have been around for years, so I'd suggest for v18 to open before moving on with the removal. I was wondering for a bit about how tss_htup could be abused in the open, and the only references I can see come from forks of the pre-2019 area, where this uses TidNext(). As a whole, ripping it out does not stress me much. -- Michael signature.asc Description: PGP signature
datfrozen/relfrozen update race condition
On Tue, May 24, 2016 at 03:01:13PM -0400, Tom Lane wrote:
> Also, I notice another problem in vac_truncate_clog() now that I'm looking
> at it: it's expecting that the pg_database datfrozenxid and datminmxid
> values will hold still while it's looking at them. Since
> vac_update_datfrozenxid updates those values in-place, there's a race
> condition against VACUUMs happening in other databases. We should fetch
> those values into local variables before doing the various tests inside
> the scan loop.
Commit 2d2e40e fixed the above. There's another problem just like it, one
layer lower. vac_update_datfrozenxid() has:
if (TransactionIdPrecedes(classForm->relfrozenxid,
newFrozenXid))
newFrozenXid = classForm->relfrozenxid;
classForm points to buffer memory, and vac_update_relstats() inplace-updates
the buffer. Like vac_truncate_clog(), we don't mind using an old value, but
those two lines must use the same value. The attached test case shows this
bug making datfrozenxid move ahead of relfrozenxid. The attached patch fixes
it. (I noticed this while finishing up patches for the heap_inplace_update
writer race in https://postgr.es/m/20231102030915.d3.nmi...@google.com.)
I audited other read-only use of inplace-updated fields. Others look safe,
because they hold rel locks that exclude VACUUM, or they make only
non-critical decisions. Still, let's change some to the load-once style, to
improve the chance of future copy/paste finding the safe style. I'm attaching
a patch for that, too. I didn't add "volatile", because I couldn't think of
how we'd care if the load moved earlier.
Author: Noah Misch
Commit: Noah Misch
Test datfrozenxid/relfrozenxid update race condition
Not for commit, mostly because the injection point won't make sense
after the fix; it would be obvious that the test is unlikely to detect
similar future bugs. (A commit-ready test would also need to remove
temporary WARNING messages added to clarify what's happening, and it
would need to hide outputs sensitive to exact XID consumption.)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index b589279..73f0245 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -53,9 +53,11 @@
#include "storage/proc.h"
#include "storage/procarray.h"
#include "utils/acl.h"
+#include "utils/builtins.h"
#include "utils/fmgroids.h"
#include "utils/guc.h"
#include "utils/guc_hooks.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
@@ -1620,6 +1622,8 @@ vac_update_datfrozenxid(void)
while ((classTup = systable_getnext(scan)) != NULL)
{
Form_pg_class classForm = (Form_pg_class) GETSTRUCT(classTup);
+ TransactionId before, after;
+ before = classForm->relfrozenxid;
/*
* Only consider relations able to hold unfrozen XIDs (anything
else
@@ -1662,7 +1666,11 @@ vac_update_datfrozenxid(void)
/* determine new horizon */
if (TransactionIdPrecedes(classForm->relfrozenxid,
newFrozenXid))
+ {
+ if (namestrcmp(>relname,
"pg_test_last_table") == 0)
+
INJECTION_POINT("between-relfrozenxid-reads");
newFrozenXid = classForm->relfrozenxid;
+ }
}
if (MultiXactIdIsValid(classForm->relminmxid))
@@ -1678,6 +1686,12 @@ vac_update_datfrozenxid(void)
if (MultiXactIdPrecedes(classForm->relminmxid,
newMinMulti))
newMinMulti = classForm->relminmxid;
}
+
+ after = classForm->relfrozenxid;
+ elog(WARNING, "done reading %s before=%u after=%u myxid=%u
myxmin=%u",
+NameStr(classForm->relname),
+before, after,
+MyProc->xid, MyProc->xmin);
}
/* we're done with pg_class */
diff --git a/src/backend/utils/adt/lockfuncs.c
b/src/backend/utils/adt/lockfuncs.c
index 13009cc..bb2db58 100644
--- a/src/backend/utils/adt/lockfuncs.c
+++ b/src/backend/utils/adt/lockfuncs.c
@@ -17,8 +17,11 @@
#include "funcapi.h"
#include "miscadmin.h"
#include "storage/predicate_internals.h"
+#include "storage/proc.h"
+#include "storage/procarray.h"
#include "utils/array.h"
#include "utils/builtins.h"
+#include "utils/wait_event.h"
/*
@@ -606,8 +609,9 @@ pg_safe_snapshot_blocking_pids(PG_FUNCTION_ARGS)
*
* Check if specified PID is blocked by any of the PIDs listed in the second
* argument. Currently, this looks for blocking caused by waiting for
- * heavyweight locks or safe snapshots. We ignore blockage caused by PIDs
- * not directly under the isolationtester's
Re: GUC-ify walsender MAX_SEND_SIZE constant
On Mon, Apr 22, 2024 at 03:40:01PM +0200, Majid Garoosi wrote: > Any news, comments, etc. about this thread? FWIW, I'd still be in favor of doing a GUC-ification of this part, but at this stage I'd need more time to do a proper study of a case where this shows benefits to prove your point, or somebody else could come in and show it. Andres has objected to this change, on the ground that this was not worth it, though you are telling the contrary. I would be curious to hear from others, first, so as we gather more opinions to reach a consensus. -- Michael signature.asc Description: PGP signature
Re: Direct SSL connection with ALPN and HBA rules
On 22/04/2024 10:47, Heikki Linnakangas wrote:
On 22/04/2024 10:19, Michael Paquier wrote:
On Sat, Apr 20, 2024 at 12:43:24AM +0300, Heikki Linnakangas wrote:
On 19/04/2024 19:48, Jacob Champion wrote:
On Fri, Apr 19, 2024 at 6:56 AM Heikki Linnakangas wrote:
With direct SSL negotiation, we always require ALPN.
(As an aside: I haven't gotten to test the version of the patch that
made it into 17 yet, but from a quick glance it looks like we're not
rejecting mismatched ALPN during the handshake as noted in [1].)
Ah, good catch, that fell through the cracks. Agreed, the client should
reject a direct SSL connection if the server didn't send ALPN. I'll add that
to the Open Items so we don't forget again.
Would somebody like to write a patch for that? I'm planning to look
at this code more closely, as well.
I plan to write the patch later today.
Here's the patch for that. The error message is:
"direct SSL connection was established without ALPN protocol negotiation
extension"
That's accurate, but I wonder if we could make it more useful to a user
who's wondering what went wrong. I'd imagine that if the server doesn't
support ALPN, it's because you have some kind of a (not necessarily
malicious) generic SSL man-in-the-middle that doesn't support it. Or
you're trying to connect to an HTTPS server. Suggestions welcome.
--
Heikki Linnakangas
Neon (https://neon.tech)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index ec20e3f3a9..f9156e29ca 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -3524,6 +3524,13 @@ keep_going: /* We will come back to here until there is
pollres = pqsecure_open_client(conn);
if (pollres == PGRES_POLLING_OK)
{
+ /* ALPN is mandatory with direct SSL negotiation */
+ if (conn->current_enc_method == ENC_DIRECT_SSL && !conn->ssl_alpn_used)
+ {
+ libpq_append_conn_error(conn, "direct SSL connection was established without ALPN protocol negotiation extension");
+ CONNECTION_FAILED();
+ }
+
/*
* At this point we should have no data already buffered.
* If we do, it was received before we performed the SSL
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index e7a4d006e1..8df3c751db 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1585,6 +1585,31 @@ open_client_SSL(PGconn *conn)
}
}
+ /* Get the protocol selected by ALPN */
+ {
+ const unsigned char *selected;
+ unsigned int len;
+
+ SSL_get0_alpn_selected(conn->ssl, , );
+
+ /* If ALPN is used, check that we negotiated the expected protocol */
+ if (selected != NULL)
+ {
+ if (len == strlen(PG_ALPN_PROTOCOL) &&
+memcmp(selected, PG_ALPN_PROTOCOL, strlen(PG_ALPN_PROTOCOL)) == 0)
+ {
+conn->ssl_alpn_used = true;
+ }
+ else
+ {
+/* shouldn't happen */
+libpq_append_conn_error(conn, "SSL connection was established with unexpected ALPN protocol");
+pgtls_close(conn);
+return PGRES_POLLING_FAILED;
+ }
+ }
+ }
+
/*
* We already checked the server certificate in initialize_SSL() using
* SSL_CTX_set_verify(), if root.crt exists.
@@ -1632,6 +1657,7 @@ pgtls_close(PGconn *conn)
conn->ssl = NULL;
conn->ssl_in_use = false;
conn->ssl_handshake_started = false;
+ conn->ssl_alpn_used = false;
destroy_needed = true;
}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 3691e5ee96..a6792917cf 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -569,6 +569,7 @@ struct pg_conn
bool ssl_handshake_started;
bool ssl_cert_requested; /* Did the server ask us for a cert? */
bool ssl_cert_sent; /* Did we send one in reply? */
+ bool ssl_alpn_used; /* Did we negotiate a protocol with TLS ALPN? */
#ifdef USE_SSL
#ifdef USE_OPENSSL
Re: POC: GROUP BY optimization
Hi! On Thu, Apr 18, 2024 at 1:57 PM Alexander Korotkov wrote: > Thank you for the fixes you've proposed. I didn't look much into > details yet, but I think the main concern Tom expressed in [1] is > whether the feature is reasonable at all. I think at this stage the > most important thing is to come up with convincing examples showing > how huge performance benefits it could cause. I will return to this > later today and will try to provide some convincing examples. I took a fresh look at 0452b461b, and have the following thoughts: 1) Previously, preprocess_groupclause() tries to reorder GROUP BY clauses to match the required ORDER BY order. It only reorders if GROUP BY pathkeys are the prefix of ORDER BY pathkeys or vice versa. So, both of them need to be satisfied by one ordering. 0452b461b also tries to match GROUP BY clauses to ORDER BY clauses, but takes into account an incremental sort. Actually, instead of removing preprocess_groupclause(), we could just teach it to take incremental sort into account. 2) The get_useful_group_keys_orderings() function takes into account 3 orderings of pathkeys and clauses: original order as written in GROUP BY, matching ORDER BY clauses as much as possible, and matching the input path as much as possible. Given that even before 0452b461b, preprocess_groupclause() could change the original order of GROUP BY clauses, so we don't need to distinguish the first two. We could just consider output of new preprocess_groupclause() taking into account an incremental sort and the ordering matching the input path as much as possible. This seems like significant simplification. Let me also describe my thoughts about the justification of the feature itself. As Tom pointed upthread, Sort + Grouping is generally unlikely faster than Hash Aggregate. The main point of this feature is being able to match the order of GROUP BY clauses to the order of the input path. That input path could be Index Scan or Subquery. Let's concentrate on Index Scan. Index Scan can give us the required order, so we can do grouping without Sort or with significantly cheaper Incremental Sort. That could be much faster than Hash Aggregate. But if we scan the full table (or its significant fraction), Index Scan is typically much more expensive than Sequential Scan because of random page access. However, there are cases when Index Scan could be cheaper. 1) If the heap row is wide and the index contains all the required columns, Index Only Scan can be cheaper than Sequential Scan because of lesser volume. 2) If the query predicate is selective enough and matches the index, Index Scan might be significantly cheaper. One may say that if the query predicate is selective enough then there are not that many matching rows, so aggregating them either way isn't a big deal anyway. However, given that data volumes are growing tremendously, it's not hard to imagine that the index selected a small fraction of table rows, but they are still enough to be costly for aggregating. Therefore, I think there are significant cases where matching GROUP BY clauses to the order of the input path could give a substantial improvement over Hash Aggregate. While there are some particular use-cases by Jian He, I hope that above could give some rationale. -- Regards, Alexander Korotkov
Re: fix tablespace handling in pg_combinebackup
On Mon, Apr 22, 2024 at 5:16 PM Thomas Munro wrote: > On Tue, Apr 23, 2024 at 8:05 AM Robert Haas wrote: > > I reworked the test cases so that they don't (I think) rely on > > symlinks working as they do on normal platforms. > > Cool. cfbot is giving me a bunch of green check marks, so I plan to commit this version, barring objections. I shall leave redesign of the symlink mess as a matter for others to ponder; I'm not in love with what we have, but I think it will be tricky to do better, and I don't think I want to spend time on it, at least not now. -- Robert Haas EDB: http://www.enterprisedb.com
Re: [Doc] Improvements to ddl.sgl Privileges Section and Glossary
Any thoughts? On Thu, Jan 25, 2024 at 1:59 PM David G. Johnston < david.g.johns...@gmail.com> wrote: > Hey, > > In a nearby user complaint email [1] some missing information regarding > ownership reassignment came to light. I took that and went a bit further > to add what I felt was further missing information and context for how the > privilege system is designed. I've tried to formalize and label existing > concepts a bit and updated the glossary accordingly. > > The attached is a partial rewrite of the patch on the linked post after > those comments were taken into consideration. > > The new glossary entry for privileges defines various qualifications of > the term that are used in the new prose. I've marked up each of the > variants and point them all back to the main entry. I didn't try to > incorporate those terms, or even really look, anywhere else in the > documentation. If the general idea is accepted that kind of work can be > done as a follow-up. > > David J. > > [1] > https://www.postgresql.org/message-id/d294818d12280f6223ddf169ab5454927f5186b6.camel%40cybertec.at > >
Re: fix tablespace handling in pg_combinebackup
Hi, On 2024-04-23 09:15:27 +1200, Thomas Munro wrote: > I find myself wondering if symlinks should go on the list of "things > we pretended Windows had out of convenience, that turned out to be > more inconvenient than we expected, and we'd do better to tackle > head-on with a more portable idea". Yes, I think the symlink design was pretty clearly a mistake. > Perhaps we could just use a tablespace map file instead to do our own path > construction, or something like that. I suspect that would be one of those > changes that is technically easy, but community-wise hard as it affects a > load of backup tools and procedures...) Yea. I wonder if we could do a somewhat transparent upgrade by creating a file alongside each tablespace that contains the path or such. Greetings, Andres
Re: [Doc] Improve hostssl related descriptions and option presentation
Thoughts anyone? On Thu, Feb 1, 2024 at 3:47 PM David G. Johnston wrote: > Motivated by a recent complaint [1] I found the hostssl related material > in our docs quite verbose and even repetitive. Some of that is normal > since we have both an overview/walk-through section as well as a reference > section. But the overview in particular was self-repetitive. Here is a > first pass at some improvements. The commit message contains both the > intent of the patch and some thoughts/questions still being considered. > > David J. > > [1] > https://www.postgresql.org/message-id/170672433664.663.11895343120533141715%40wrigleys.postgresql.org >
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
On Mon, 22 Apr 2024 at 16:26, Robert Haas wrote: > That's a fair point, but I'm still not seeing much practical > advantage. It's unlikely that a client is going to set a random bit in > a format parameter for no reason. I think you're missing an important point of mine here. The client wouldn't be "setting a random bit in a format parameter for no reason". The client would decide it is allowed to set this bit, because the PG version it connected to supports column encryption (e.g. PG18). But this completely breaks protocol and application layer separation. It doesn't seem completely outside of the realm of possibility for a pooler to gather some statistics on the amount of Bind messages that use text vs binary query parameters. That's very easily doable now, while looking only at the protocol layer. If a client then sets the new format parameter bit, this pooler could then get confused and close the connection. > Perhaps this is the root of our disagreement, or at least part of it. > I completely agree that it is important for human beings to be able to > understand whether, and how, the wire protocol has changed from one > release to another. I think this is partially the reason for our disagreement, but I think there are at least two other large reasons: 1. I strongly believe minor protocol version bumps after the initial 3.1 one can be made painless for clients/poolers (so the ones to 3.2/3.3/etc). Similar to how TLS 1.3 can be safely introduced, and not having to worry about breaking TLS 1.2 communication. Once clients and poolers implement version negotiation support for 3.1, there's no reason for version negation support to work for 3.0 and 3.1 to then suddenly break on the 3.2 bump. To be clear, I'm talking about the act of bumping the version here, not the actual protocol changes. So assuming zero/near-zero client implementation effort for the new features (like never setting the newly supported bit in a format parameter), then bumping the protocol version for these new features can never have negative consequences. 2. I very much want to keep a clear split between the protocol layer and the application layer of our communication. And these layers merge whenever (like you say) "the wire protocol has changed from one release to another", but no protocol version bump or protocol extension is used to indicate that. When that happens the only way for a client to know what valid wire protocol messages are according to the server, is by checking the server version. This completely breaks the separation between layers. So, while checking the server version indeed works for direct client to postgres communication, it starts to break down whenever you put a pooler inbetween (as explained in the example earlier in this email). And it breaks down even more when connecting to servers that implement the Postgres wire protocol, but are not postgres at all, like CockroachDB. Right now libpq and other postgres drivers can be used to talk to these other servers and poolers, but if we start mixing protocol and application layer stuff then eventually that will stop being the case. Afaict from your responses, you disagree with 1. However, it's not at all clear to me what exact problems you're worried about. It sounds like you don't know either, and it's more that you're worried about things breaking for not yet known reasons. I hoped to take away/reduce those worries using some arguments in a previous email (quoted below), but you didn't respond to those arguments, so I'm not sure if they were able to change your mind. On Thu, 18 Apr 2024 at 21:34, Jelte Fennema-Nio wrote: > When the server supports a lower version than the client, the client > should disable certain features because it gets the > ProtocolVersionNegotiation message. This is also true if we don't bump > the version. Negotiating a lower version actually makes it clearer for > the client what features to disable. Using the reported postgres > version for this might not, because a connection pooler in the middle > might not support the features that the client wants and thus throw an > error (e.g. due to the client sending unknown messages) even if the > backing Postgres server would support these features. Not to mention > non-postgresql servers that implement the PostgreSQL protocol (of > which there are more and more). > > When the server supports a higher version, the client never even > notices this because the server will silently accept and only enable > the features of the lower version. So this could never cause breakage, > as from the client's perspective the server didn't bump their protocol > version.
Re: fix tablespace handling in pg_combinebackup
On Tue, Apr 23, 2024 at 8:05 AM Robert Haas wrote: > I reworked the test cases so that they don't (I think) rely on > symlinks working as they do on normal platforms. Cool. (It will remain a mystery for now why perl readlink() can't read the junction points that PostgreSQL creates (IIUC), but the OS can follow them and PostgreSQL itself can read them with apparently similar code. I find myself wondering if symlinks should go on the list of "things we pretended Windows had out of convenience, that turned out to be more inconvenient than we expected, and we'd do better to tackle head-on with a more portable idea". Perhaps we could just use a tablespace map file instead to do our own path construction, or something like that. I suspect that would be one of those changes that is technically easy, but community-wise hard as it affects a load of backup tools and procedures...)
Re: DROP OWNED BY fails to clean out pg_init_privs grants
> On 21 Apr 2024, at 23:08, Tom Lane wrote: > ... were you going to look at it? I can take a whack if it's too far down > your priority list. Yeah, I’m working on a patchset right now. ./daniel
Re: Why does pgindent's README say to download typedefs.list from the buildfarm?
Nathan Bossart writes: > On Mon, Apr 22, 2024 at 04:08:08PM -0400, Tom Lane wrote: >> I think the actual plan now is that we'll sync the in-tree copy >> with the buildfarm's results (and then do a tree-wide pgindent) >> every so often, probably shortly before beta every year. > Okay. Is this just to resolve the delta between the manual updates and a > clean autogenerated copy every once in a while? The main reason there's a delta is that people don't manage to maintain the in-tree copy perfectly (at least, they certainly haven't done so for this past year). So we need to do that to clean up every now and then. A secondary reason is that the set of typedefs we absorb from system include files changes over time. regards, tom lane
Re: Why does pgindent's README say to download typedefs.list from the buildfarm?
On Mon, Apr 22, 2024 at 04:08:08PM -0400, Tom Lane wrote: > I think the actual plan now is that we'll sync the in-tree copy > with the buildfarm's results (and then do a tree-wide pgindent) > every so often, probably shortly before beta every year. Okay. Is this just to resolve the delta between the manual updates and a clean autogenerated copy every once in a while? > The problem with the README is that it describes that process, > rather than the now-typical workflow of incrementally keeping > the tree indented. I don't think we want to remove the info > about how to do the full-monty process, but you're right that > the README needs to explain the incremental method as being > the one most developers would usually use. > > Want to write some text? Yup, I'll put something together. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Statistics Import and Export
Jeff Davis writes: > Would it make sense to have a new SECTION_STATS? Perhaps, but the implications for pg_dump's API would be nontrivial, eg would we break any applications that know about the current options for --section. And you still have to face up to the question "does --data-only include this stuff?". > Philosophically, I suppose stats are data, but I still don't understand > why considering stats to be data is so important in pg_dump. > Practically, I want to dump stats XOR data. That's because, if I dump > the data, it's so costly to reload and rebuild indexes that it's not > very important to avoid a re-ANALYZE. Hmm, interesting point. But the counterargument to that is that the cost of building indexes will also dwarf the cost of installing stats, so why not do so? Loading data without stats, and hoping that auto-analyze will catch up sooner not later, is exactly the current behavior that we're doing all this work to get out of. I don't really think we want it to continue to be the default. regards, tom lane
Re: Why does pgindent's README say to download typedefs.list from the buildfarm?
Nathan Bossart writes: > I used to do this step when I first started hacking on Postgres because > that's what it says to do, but I've only ever used the in-tree one for many > years now, and I'm not aware of any scenario where I might need to download > a new version from the buildfarm. I see that the in-tree copy wasn't added > until 2010 (commit 1604057), so maybe this is just leftover from back then. > Could we remove this note now? I think the actual plan now is that we'll sync the in-tree copy with the buildfarm's results (and then do a tree-wide pgindent) every so often, probably shortly before beta every year. The problem with the README is that it describes that process, rather than the now-typical workflow of incrementally keeping the tree indented. I don't think we want to remove the info about how to do the full-monty process, but you're right that the README needs to explain the incremental method as being the one most developers would usually use. Want to write some text? regards, tom lane
Re: fix tablespace handling in pg_combinebackup
I reworked the test cases so that they don't (I think) rely on symlinks working as they do on normal platforms. Here's a patch. I'll go create a CommitFest entry for this thread so that cfbot will look at it. ...Robert v1-0001-Try-again-to-add-test-coverage-for-pg_combineback.patch Description: Binary data
Why does pgindent's README say to download typedefs.list from the buildfarm?
I used to do this step when I first started hacking on Postgres because that's what it says to do, but I've only ever used the in-tree one for many years now, and I'm not aware of any scenario where I might need to download a new version from the buildfarm. I see that the in-tree copy wasn't added until 2010 (commit 1604057), so maybe this is just leftover from back then. Could we remove this note now? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
doc: create table improvements
Hey,
The attached patch addresses four somewhat related aspects of the create
table reference page that bother me.
This got started with Bug# 15954 [1] (unlogged on a partitioned table
doesn't make sense) and I've added a paragraph under "unlogged" to address
it.
While doing that, it seemed desirable to explicitly frame up both temporary
and unlogged as being "persistence modes" so I added a mention of both in
the description. Additionally, it seemed appropriate to do so immediately
after the opening paragraph since the existing second paragraph goes
immediately into talking about temporary tables and schemas. I figured a
link to the reliability chapter where one learns about WAL and why/how
these alternative persistence modes exist is worthwhile. (I added a missing
comma to the first sentence while I was in the area)
Third, I've had a long-standing annoyance regarding the excessive length of
the CREATE line of each of the create table syntax blocks. Replacing the
syntax for the persistence modes with a named placeholder introduces
structure and clarity while reducing the length of the line nicely.
Finally, while fixing line lengths, the subsequent line (first form) for
column specification is even more excessive. Pulling out the
column_storage syntax into a named reference nicely cleans this line up.
David J.
P.S. I didn't go into depth on the fact the persistence options are not
inherited/copied/like-able; so for now the fact they are not so is
discovered by their omission when discussing those topics.
[1]
https://www.postgresql.org/message-id/flat/15954-b61523bed4b110c4%40postgresql.org
From e375044d55809d239be33f31c4efa8410790d3f0 Mon Sep 17 00:00:00 2001
From: "David G. Johnston"
Date: Mon, 22 Apr 2024 11:51:53 -0700
Subject: [PATCH] doc: create table doc enhancements
---
doc/src/sgml/ref/create_table.sgml | 37 +-
1 file changed, 31 insertions(+), 6 deletions(-)
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 02f31d2d6f..9a5dafb9af 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -21,8 +21,8 @@ PostgreSQL documentation
-CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXISTS ] table_name ( [
- { column_name data_type [ STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN | DEFAULT } ] [ COMPRESSION compression_method ] [ COLLATE collation ] [ column_constraint [ ... ] ]
+CREATE [ persistence_mode ] TABLE [ IF NOT EXISTS ] table_name ( [
+ { column_name data_type [ column_storage ] [ COLLATE collation ] [ column_constraint [ ... ] ]
| table_constraint
| LIKE source_table [ like_option ... ] }
[, ... ]
@@ -34,7 +34,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
[ ON COMMIT { PRESERVE ROWS | DELETE ROWS | DROP } ]
[ TABLESPACE tablespace_name ]
-CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXISTS ] table_name
+CREATE [ persistence_mode ] TABLE [ IF NOT EXISTS ] table_name
OF type_name [ (
{ column_name [ WITH OPTIONS ] [ column_constraint [ ... ] ]
| table_constraint }
@@ -46,7 +46,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
[ ON COMMIT { PRESERVE ROWS | DELETE ROWS | DROP } ]
[ TABLESPACE tablespace_name ]
-CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXISTS ] table_name
+CREATE [ persistence_mode ] TABLE [ IF NOT EXISTS ] table_name
PARTITION OF parent_table [ (
{ column_name [ WITH OPTIONS ] [ column_constraint [ ... ] ]
| table_constraint }
@@ -58,7 +58,16 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
[ ON COMMIT { PRESERVE ROWS | DELETE ROWS | DROP } ]
[ TABLESPACE tablespace_name ]
-where column_constraint is:
+where persistence_mode is:
+
+[ GLOBAL | LOCAL ] { TEMPORARY | TEMP } |
+UNLOGGED
+
+and column_storage is:
+
+STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN | DEFAULT } [ COMPRESSION compression_method ]
+
+and column_constraint is:
[ CONSTRAINT constraint_name ]
{ NOT NULL |
@@ -118,11 +127,21 @@ WITH ( MODULUS numeric_literal, REM
Description
- CREATE TABLE will create a new, initially empty table
+ CREATE TABLE will create a new, initially empty, table
in the current database. The table will be owned by the user issuing the
command.
+
+ The reliability characteristics of a table are governed by its
+ persistence mode. The default mode is described
+ here
+ There are two alternative modes that can be specified during
+ table creation:
+ temporary and
+ unlogged.
+
+
If a schema name is given (for example, CREATE TABLE
myschema.mytable ...) then the table is created in the specified
@@ -221,6 +240,12 @@ WITH ( MODULUS numeric_literal, REM
If this is specified, any sequences created together with the unlogged
table (for
Re: post-freeze damage control
On Tue, Apr 16, 2024 at 7:20 PM Jeff Davis wrote: > We maintain the invariant: > >XLogCtl->logFlushResult <= XLogCtl->logWriteResult > > and the non-shared version: > >LogwrtResult.Flush <= LogwrtResult.Write > > and that the requests don't fall behind the results: I had missed the fact that this commit added a bunch of read and write barriers; and I think that's the main reason why I was concerned. The existence of those barriers, and the invariants they are intended to maintain, makes me feel better about it. Thanks for writing back about this! -- Robert Haas EDB: http://www.enterprisedb.com
Re: improve performance of pg_dump --binary-upgrade
I noticed that there are some existing examples of this sort of thing in
pg_dump (e.g., commit d5e8930), so I adjusted the patch to match the
surrounding style a bit better.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 80dd3233730422298ffe3b4523a7c58d7e9b55b9 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Thu, 18 Apr 2024 15:15:19 -0500
Subject: [PATCH v4 1/2] Remove is_index parameter from
binary_upgrade_set_pg_class_oids().
---
src/bin/pg_dump/pg_dump.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index ed9bab3bfe..bbdb0628c9 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -324,7 +324,7 @@ static void binary_upgrade_set_type_oids_by_rel(Archive *fout,
const TableInfo *tbinfo);
static void binary_upgrade_set_pg_class_oids(Archive *fout,
PQExpBuffer upgrade_buffer,
- Oid pg_class_oid, bool is_index);
+ Oid pg_class_oid);
static void binary_upgrade_extension_member(PQExpBuffer upgrade_buffer,
const DumpableObject *dobj,
const char *objtype,
@@ -5394,8 +5394,7 @@ binary_upgrade_set_type_oids_by_rel(Archive *fout,
static void
binary_upgrade_set_pg_class_oids(Archive *fout,
- PQExpBuffer upgrade_buffer, Oid pg_class_oid,
- bool is_index)
+ PQExpBuffer upgrade_buffer, Oid pg_class_oid)
{
PQExpBuffer upgrade_query = createPQExpBuffer();
PGresult *upgrade_res;
@@ -5444,7 +5443,8 @@ binary_upgrade_set_pg_class_oids(Archive *fout,
appendPQExpBufferStr(upgrade_buffer,
"\n-- For binary upgrade, must preserve pg_class oids and relfilenodes\n");
- if (!is_index)
+ if (relkind != RELKIND_INDEX &&
+ relkind != RELKIND_PARTITIONED_INDEX)
{
appendPQExpBuffer(upgrade_buffer,
"SELECT pg_catalog.binary_upgrade_set_next_heap_pg_class_oid('%u'::pg_catalog.oid);\n",
@@ -11878,7 +11878,7 @@ dumpCompositeType(Archive *fout, const TypeInfo *tyinfo)
binary_upgrade_set_type_oids_by_type_oid(fout, q,
tyinfo->dobj.catId.oid,
false, false);
- binary_upgrade_set_pg_class_oids(fout, q, tyinfo->typrelid, false);
+ binary_upgrade_set_pg_class_oids(fout, q, tyinfo->typrelid);
}
qtypname = pg_strdup(fmtId(tyinfo->dobj.name));
@@ -16012,7 +16012,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
if (dopt->binary_upgrade)
binary_upgrade_set_pg_class_oids(fout, q,
- tbinfo->dobj.catId.oid, false);
+ tbinfo->dobj.catId.oid);
appendPQExpBuffer(q, "CREATE VIEW %s", qualrelname);
@@ -16114,7 +16114,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
if (dopt->binary_upgrade)
binary_upgrade_set_pg_class_oids(fout, q,
- tbinfo->dobj.catId.oid, false);
+ tbinfo->dobj.catId.oid);
appendPQExpBuffer(q, "CREATE %s%s %s",
tbinfo->relpersistence == RELPERSISTENCE_UNLOGGED ?
@@ -17005,7 +17005,7 @@ dumpIndex(Archive *fout, const IndxInfo *indxinfo)
if (dopt->binary_upgrade)
binary_upgrade_set_pg_class_oids(fout, q,
- indxinfo->dobj.catId.oid, true);
+ indxinfo->dobj.catId.oid);
/* Plain secondary index */
appendPQExpBuffer(q, "%s;\n", indxinfo->indexdef);
@@ -17259,7 +17259,7 @@ dumpConstraint(Archive *fout, const ConstraintInfo *coninfo)
if (dopt->binary_upgrade)
binary_upgrade_set_pg_class_oids(fout, q,
- indxinfo->dobj.catId.oid, true);
+ indxinfo->dobj.catId.oid);
appendPQExpBuffer(q, "ALTER %sTABLE ONLY %s\n", foreign,
fmtQualifiedDumpable(tbinfo));
@@ -17668,7 +17668,7 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo)
if (dopt->binary_upgrade)
{
binary_upgrade_set_pg_class_oids(fout, query,
- tbinfo->dobj.catId.oid, false);
+ tbinfo->dobj.catId.oid);
/*
* In older PG versions a sequence will have a pg_type entry, but v14
--
2.25.1
>From 14da726b675e4dd3e69f0b75f256b2757e22b900 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Mon, 22 Apr 2024 13:21:18 -0500
Subject: [PATCH v4 2/2] Improve performance of pg_dump --binary-upgrade.
---
src/bin/pg_dump/pg_dump.c| 141 +--
src/tools/pgindent/typedefs.list | 1 +
2 files changed, 96 insertions(+), 46 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index bbdb0628c9..cad391dec1 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -55,6 +55,7 @@
#include "catalog/pg_trigger_d.h"
#include "catalog/pg_type_d.h"
#include "common/connect.h"
+#include "common/int.h"
#include "common/relpath.h"
#include "compress_io.h"
#include "dumputils.h"
@@ -92,6 +93,17 @@ typedef struct
int objsubid; /* subobject (table column #) */
} SecLabelItem;
+typedef struct
+{
+ Oid oid; /* object OID */
+ char relkind; /* object kind */
+ RelFileNumber relfilenode; /*
Re: Add notes to pg_combinebackup docs
On Thu, Apr 18, 2024 at 3:25 PM Daniel Gustafsson wrote: > > On 18 Apr 2024, at 20:11, Robert Haas wrote: > > 2. As (1), but make check_control_files() emit a warning message when > > the problem case is detected. > > Being in the post-freeze part of the cycle, this seems like the best option. Here is a patch for that. -- Robert Haas EDB: http://www.enterprisedb.com v1-0001-pg_combinebackup-Detect-checksum-mismatches-and-d.patch Description: Binary data
Re: Optimizing nbtree ScalarArrayOp execution, allowing multi-column ordered scans, skip scan
On Mon, Apr 22, 2024 at 11:13 AM Peter Geoghegan wrote: > I'm pretty sure that I could fix this by simply removing the > assertion. But I need to think about it a bit more before I push a > fix. > > The test case you've provided proves that _bt_preprocess_keys's > new no-op path isn't just used during scans that have array keys (your > test case doesn't have a SAOP at all). This was never intended. On the > other hand, I think that it's still correct (or will be once the assertion is > gone), and it seems like it would be simpler to allow this case (and > document it) than to not allow it at all. Pushed a fix like that just now. Thanks for the report, Alexander. -- Peter Geoghegan
Re: Statistics Import and Export
On Wed, 2024-04-17 at 11:50 -0500, Nathan Bossart wrote: > It looks like the problem is that the ACLs are getting dumped in the > data > section when we are also dumping stats. I'm able to get the tests to > pass > by moving the call to dumpRelationStats() that's in dumpTableSchema() > to > dumpTableData(). I'm not entirely sure why that fixes it yet, but if > we're > treating stats as data, then it intuitively makes sense for us to > dump it > in dumpTableData(). Would it make sense to have a new SECTION_STATS? > However, that seems to prevent the stats from getting > exported in the --schema-only/--binary-upgrade scenario, which > presents a > problem for pg_upgrade. ISTM we'll need some extra hacks to get this > to > work as desired. Philosophically, I suppose stats are data, but I still don't understand why considering stats to be data is so important in pg_dump. Practically, I want to dump stats XOR data. That's because, if I dump the data, it's so costly to reload and rebuild indexes that it's not very important to avoid a re-ANALYZE. Regards, Jeff Davis
Re: subscription/026_stats test is intermittently slow?
Hi, On 2024-04-19 13:57:41 -0400, Robert Haas wrote: > Have others seen this? Is there something we can/should do about it? Yes, I've also seen this - but never quite reproducible enough to properly tackle it. The first thing I'd like to do is to make the wait_for_catchup routine regularly log the current state, so we can in retrospect analyze e.g. whether there was continual, but slow, replay progress, or whether replay was entirely stuck. wait_for_catchup() not being debuggable has been a problem in many different tests, so I think it's high time to fix that. Greetings, Andres Freund
Re: BitmapHeapScan streaming read user and prelim refactoring
On Thu, Apr 18, 2024 at 5:39 AM Tomas Vondra wrote: > > On 4/18/24 09:10, Michael Paquier wrote: > > On Sun, Apr 07, 2024 at 10:54:56AM -0400, Melanie Plageman wrote: > >> I've added an open item [1], because what's one open item when you can > >> have two? (me) > > > > And this is still an open item as of today. What's the plan to move > > forward here? > > AFAIK the plan is to replace the asserts with actually resetting the > rs_empty_tuples_pending field to 0, as suggested by Melanie a week ago. > I assume she was busy with the post-freeze AM reworks last week, so this > was on a back burner. yep, sorry. Also I took a few days off. I'm just catching up today. I want to pop in one of Richard or Tomas' examples as a test, since it seems like it would add some coverage. I will have a patch soon. - Melanie
Re: Cleanup: remove unused fields from nodes
On Mon, 22 Apr 2024 at 17:41, Tom Lane wrote: > > Matthias van de Meent writes: > > 0002/0004 remove fields in ExecRowMark which were added for FDWs to > > use, but there are no FDWs which use this: I could only find two FDWs > > who implement RefetchForeignRow, one being BlackHoleFDW, and the other > > a no-op implementation in kafka_fdw [0]. We also don't seem to have > > any testing on this feature. > > I'm kind of down on removing either of these. ermExtra is explicitly > intended for extensions to use, and just because we haven't found any > users doesn't mean there aren't any, or might not be next week. That's a good point, and also why I wasn't 100% sure removing it was a good idea. I'm not quite sure why this would be used (rather than the internal state of the FDW, or no state at all), but haven't looked very deep into it either, so I'm quite fine with not channging that. > Similarly, ermActive seems like it's at least potentially useful: > is there another way for onlookers to discover that state? The ermActive field is always true when RefetchForeignRow is called (in ExecLockRows(), in nodeLockRows.c), and we don't seem to care about the value of the field afterwards. Additionally, we always set erm->curCtid to a valid value when ermActive is also first set in that code path. In all, it feels like a duplicative field with no real uses inside PostgreSQL itself. If an extension (FDW) needs it, it should probably use ermExtra instead, as ermActive seemingly doesn't carry any meaningful value into the FDW call. > I think it would be a good idea to push 0003 for v17, just so nobody > grows an unnecessary dependency on that field. 0001 and 0005 could > be left for v18, but on the other hand they're so trivial that it > could also be sensible to just push them to get them out of the way. Beta 1 scheduled to be released for quite some time, so I don't think there are any problems with fixing these kinds of minor issues in the provisional ABI for v17. Kind regards, Matthias van de Meent
Re: TerminateOtherDBBackends code comments inconsistency.
On Mon, Apr 15, 2024 at 11:17:54AM +0530, Amit Kapila wrote: > On Thu, Apr 11, 2024 at 6:58 PM Kirill Reshke wrote: > > > > While working on [0] i have noticed this comment in > > TerminateOtherDBBackends function: > > > > /* > > * Check whether we have the necessary rights to terminate other > > * sessions. We don't terminate any session until we ensure that we > > * have rights on all the sessions to be terminated. These checks are > > * the same as we do in pg_terminate_backend. > > * > > * In this case we don't raise some warnings - like "PID %d is not a > > * PostgreSQL server process", because for us already finished session > > * is not a problem. > > */ > > > > This statement is not true after 3a9b18b. > > "These checks are the same as we do in pg_terminate_backend." The comment mismatch is a problem. Thanks for reporting it. The DROP DATABASE doc mimics the comment, using, "permissions are the same as with pg_terminate_backend". > > But the code is still correct, I assume... or not? In fact, we are > > killing autovacuum workers which are working with a given database > > (proc->roleId == 0), which is OK in that case. Are there any other > > cases when proc->roleId == 0 but we should not be able to kill such a > > process? > > > > Good question. I am not aware of such cases but I wonder if we should > add a check similar to 3a9b18b [1] for the reason given in the commit > message. I have added Noah to see if he has any suggestions on this > matter. > > [1] - > commit 3a9b18b3095366cd0c4305441d426d04572d88c1 > Author: Noah Misch > Date: Mon Nov 6 06:14:13 2023 -0800 > > Ban role pg_signal_backend from more superuser backend types. That commit distinguished several scenarios. Here's how they apply in TerminateOtherDBBackends(): - logical replication launcher, autovacuum launcher: the proc->databaseId test already skips them, since they don't connect to a database. Vignesh said this. - autovacuum worker: should terminate, since CountOtherDBBackends() would terminate them in DROP DATABASE without FORCE. - background workers that connect to a database: the right thing is less clear on these, but I lean toward allowing termination and changing the DROP DATABASE doc. As a bgworker author, I would value being able to recommend DROP DATABASE FORCE if a worker is sticking around unexpectedly. There's relatively little chance of a bgworker actually wanting to block DROP DATABASE FORCE or having an exploitable termination-time bug. Thoughts?
Re: slightly misleading Error message in guc.c
jian he writes:
> set work_mem to '1kB';
> ERROR: 1 kB is outside the valid range for parameter "work_mem" (64
> .. 2147483647)
> should it be
> ERROR: 1 kB is outside the valid range for parameter "work_mem" (64
> kB .. 2147483647 kB)
> ?
> since the units for work_mem are { "B", "kB", "MB", "GB", and "TB"}
Seems like a useful change ... about like this?
regards, tom lane
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index f51b3e0b50..3fb6803998 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3173,15 +3173,20 @@ parse_and_validate_value(struct config_generic *record,
if (newval->intval < conf->min || newval->intval > conf->max)
{
const char *unit = get_config_unit_name(conf->gen.flags);
+ const char *unitspace;
+
+ if (unit)
+ unitspace = " ";
+ else
+ unit = unitspace = "";
ereport(elevel,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("%d%s%s is outside the valid range for parameter \"%s\" (%d .. %d)",
- newval->intval,
- unit ? " " : "",
- unit ? unit : "",
+ errmsg("%d%s%s is outside the valid range for parameter \"%s\" (%d%s%s .. %d%s%s)",
+ newval->intval, unitspace, unit,
name,
- conf->min, conf->max)));
+ conf->min, unitspace, unit,
+ conf->max, unitspace, unit)));
return false;
}
@@ -3209,15 +3214,20 @@ parse_and_validate_value(struct config_generic *record,
if (newval->realval < conf->min || newval->realval > conf->max)
{
const char *unit = get_config_unit_name(conf->gen.flags);
+ const char *unitspace;
+
+ if (unit)
+ unitspace = " ";
+ else
+ unit = unitspace = "";
ereport(elevel,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("%g%s%s is outside the valid range for parameter \"%s\" (%g .. %g)",
- newval->realval,
- unit ? " " : "",
- unit ? unit : "",
+ errmsg("%g%s%s is outside the valid range for parameter \"%s\" (%g%s%s .. %g%s%s)",
+ newval->realval, unitspace, unit,
name,
- conf->min, conf->max)));
+ conf->min, unitspace, unit,
+ conf->max, unitspace, unit)));
return false;
}
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 127c953297..455b6d6c0c 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -510,7 +510,7 @@ SELECT '2006-08-13 12:34:56'::timestamptz;
SET seq_page_cost TO 'NaN';
ERROR: invalid value for parameter "seq_page_cost": "NaN"
SET vacuum_cost_delay TO '10s';
-ERROR: 1 ms is outside the valid range for parameter "vacuum_cost_delay" (0 .. 100)
+ERROR: 1 ms is outside the valid range for parameter "vacuum_cost_delay" (0 ms .. 100 ms)
SET no_such_variable TO 42;
ERROR: unrecognized configuration parameter "no_such_variable"
-- Test "custom" GUCs created on the fly (which aren't really an
Re: Cleanup: remove unused fields from nodes
Matthias van de Meent writes: > 0001 removes two old fields that are not in use anywhere anymore, but > at some point these were used. +1. They're not being initialized, so they're useless and confusing. > 0002/0004 remove fields in ExecRowMark which were added for FDWs to > use, but there are no FDWs which use this: I could only find two FDWs > who implement RefetchForeignRow, one being BlackHoleFDW, and the other > a no-op implementation in kafka_fdw [0]. We also don't seem to have > any testing on this feature. I'm kind of down on removing either of these. ermExtra is explicitly intended for extensions to use, and just because we haven't found any users doesn't mean there aren't any, or might not be next week. Similarly, ermActive seems like it's at least potentially useful: is there another way for onlookers to discover that state? > 0003 drops the input_finfo field on the new JsonExprState struct. It > wasn't read anywhere, so keeping it around makes little sense IMO. +1. The adjacent input_fcinfo field has this info if anyone needs it. > 0005 drops field DeallocateStmt.isall: the value of the field is > implied by !name, and that field was used as such. Seems reasonable. I think it would be a good idea to push 0003 for v17, just so nobody grows an unnecessary dependency on that field. 0001 and 0005 could be left for v18, but on the other hand they're so trivial that it could also be sensible to just push them to get them out of the way. regards, tom lane
Cleanup: remove unused fields from nodes
Hi, While working on the 'reduce nodeToString output' patch, I noticed that my IDE marked one field in the TidScanState node as 'unused'. After checking this seemed to be accurate, and I found a few more such fields in Node structs. PFA some patches that clean this up: 0001 is plain removal of fields that are not accessed anywhere anymore, 0002 and up clean up fields that are effectively write-only, with no effective use inside PostgreSQL's own code, and no effective usage found on Debian code search, nor Github code search. I'm quite confident about the correctness of patches 1 and 3 (no usage at all, and newly introduced with no meaningful uses), while patches 2, 4, and 5 could be considered 'as designed'. For those last ones I have no strong opinion for removal or against keeping them around, this is just to point out we can remove the fields, as nobody seems to be using them. /cc Tom Lane and Etsuro Fujita: 2 and 4 were introduced with your commit afb9249d in 2015. /cc Amit Kapila: 0003 was introduced with your spearheaded commit 6185c973 this year. Kind regards, Matthias van de Meent 0001 removes two old fields that are not in use anywhere anymore, but at some point these were used. 0002/0004 remove fields in ExecRowMark which were added for FDWs to use, but there are no FDWs which use this: I could only find two FDWs who implement RefetchForeignRow, one being BlackHoleFDW, and the other a no-op implementation in kafka_fdw [0]. We also don't seem to have any testing on this feature. 0003 drops the input_finfo field on the new JsonExprState struct. It wasn't read anywhere, so keeping it around makes little sense IMO. 0005 drops field DeallocateStmt.isall: the value of the field is implied by !name, and that field was used as such. [0] https://github.com/cohenjo/kafka_fdw/blob/master/src/kafka_fdw.c#L1793 v1-0002-Remove-field-ExecRowMark.ermActive.patch Description: Binary data v1-0003-Remove-field-JsonExprState.input_finfo.patch Description: Binary data v1-0005-Remove-field-DeallocateStmt.isall.patch Description: Binary data v1-0001-Remove-some-unused-fields-from-nodes-in-execnodes.patch Description: Binary data v1-0004-Remove-field-ExecRowMark.ermExtra.patch Description: Binary data
Re: Optimizing nbtree ScalarArrayOp execution, allowing multi-column ordered scans, skip scan
On Mon, Apr 22, 2024 at 4:00 AM Alexander Lakhin wrote:
> Please look at another case, where a similar Assert (but this time in
> _bt_preprocess_keys()) is triggered:
> CREATE TABLE t (a text, b text);
> INSERT INTO t (a, b) SELECT 'a', repeat('b', 100) FROM generate_series(1,
> 500) g;
> CREATE INDEX t_idx ON t USING btree(a);
> BEGIN;
> DECLARE c CURSOR FOR SELECT a FROM t WHERE a = 'a';
> FETCH FROM c;
> FETCH RELATIVE 0 FROM c;
>
> TRAP: failed Assert("so->numArrayKeys"), File: "nbtutils.c", Line: 2582, PID:
> 1130962
I'm pretty sure that I could fix this by simply removing the
assertion. But I need to think about it a bit more before I push a
fix.
The test case you've provided proves that _bt_preprocess_keys's
new no-op path isn't just used during scans that have array keys (your
test case doesn't have a SAOP at all). This was never intended. On the
other hand, I think that it's still correct (or will be once the assertion is
gone), and it seems like it would be simpler to allow this case (and
document it) than to not allow it at all.
The general idea that we only need one "real" _bt_preprocess_keys call
per btrescan (independent of the presence of array keys) still seems
sound.
--
Peter Geoghegan
Re: AIX support
On Sat Apr 20, 2024 at 10:42 AM CDT, Peter Eisentraut wrote: 3. We leave it out of PG17 and consider a new AIX port for PG18 on its own merits. Note that such a "new" port would probably require quite a bit of development and research work, to clean up all the cruft that had accumulated over the years in the old port. Another looming issue is that the meson build system only supported AIX with gcc before the removal. I don't know what it would take to expand that to support xclang, but if it requires meson upstream work, you have that to do, too. Happy to help advocate for any PRs from AIX folks on the Meson side. You can find me as @tristan957 on github. -- Tristan Partin Neon (https://neon.tech)
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
On Thu, Apr 18, 2024 at 5:36 PM Jelte Fennema-Nio wrote: > To clarify: My proposed approach is to use a protocol extension > parameter for to enable the new messages that the server can send > (like Peter is doing now). And **in addition to that** gate the new > Bind format type behind a feature switch. There is literally nothing > clients will have to do to "support" that feature (except for > requesting a higher version protocol). Your suggestion of not bumping > the version but still allowing the new format type on version 3.0 > doesn't have any advantage afaict, except secretly hiding from any > pooler in the middle that such a format type might be sent. That's a fair point, but I'm still not seeing much practical advantage. It's unlikely that a client is going to set a random bit in a format parameter for no reason. > As a connection pooler maintainer I would definitely love it if every > protocol change required either a protocol version parameter or a > protocol version bump. That way I can easily check every release if > the protocol changed by looking at two things, instead of diffing the > protocol docs for some tiny "supposedly irrelevant" change was made. Perhaps this is the root of our disagreement, or at least part of it. I completely agree that it is important for human beings to be able to understand whether, and how, the wire protocol has changed from one release to another. I think it would be useful to document that, and maybe some agreement to start actually bumping the version number would come out of that, either immediately or eventually. But I don't think bumping the protocol version first is going to help anything. If you know that something has changed at least one time in the release, you still have to figure out what it was, and whether there were any more of them that, presumably, would not bump the protocol version because there would be no good reason to do that more than once per major release. Not only that, but it's entirely possible that someone could fail to realize that they were supposed to bump the protocol version, or have some reason not to do it in a particular instance, so even if there are no bumps at all in a particular release cycle, that doesn't prove that there are no changes that you would have liked to know about. Said differently, I think bumping the protocol version should be, first and foremost, a way of telling the computer on the end of the connection something that it needs to know. There is a separate problem of making sure that human maintainers know what they need to know, and I think we're doing that quite poorly right now, but I think you might be conflating those two problems a bit. -- Robert Haas EDB: http://www.enterprisedb.com
RE: Slow catchup of 2PC (twophase) transactions on replica in LR
Dear Hayato, On Monday, April 22, 2024 15:54 MSK, "Hayato Kuroda (Fujitsu)" wrote: > Dear Vitaly, > > > I looked at the patch and realized that I can't try it easily in the near > > future > > because the solution I'm working on is based on PG16 or earlier. This patch > > is > > not easily applicable to the older releases. I have to port my solution to > > the > > master, which is not done yet. > > We also tried to port our patch for PG16, but the largest barrier was that a > replication command ALTER_SLOT is not supported. Since the slot option > two_phase > can't be modified, it is difficult to skip decoding PREPARE command even when > altering the option from true to false. > IIUC, Adding a new feature (e.g., replication command) for minor updates is > generally > prohibited > ... Attached patch set is a ported version for PG16, which breaks ABI. This can be used for testing purpose, but it won't be pushed to REL_16_STABLE. At least, this patchset can pass my github CI. Can you apply and check whether your issue is solved?It is fantastic. Thank you for your help! I will definitely try your patch. I need some time to test and incorporate it. I also plan to port my stuff to the master branch to simplify testing of patches. With best regards, Vitaly Davydov
Re: allow changing autovacuum_max_workers without restarting
On Fri, Apr 19, 2024 at 4:29 PM Nathan Bossart wrote: > I certainly don't want to hold up $SUBJECT for a larger rewrite of > autovacuum scheduling, but I also don't want to shy away from a larger > rewrite if it's an idea whose time has come. I'm looking forward to > hearing your ideas in your pgconf.dev talk. Yeah, I suppose I was hoping you were going to tell me the all the answers and thus make the talk a lot easier to write, but I guess life isn't that simple. :-) -- Robert Haas EDB: http://www.enterprisedb.com
Re: slightly misleading Error message in guc.c
Hi,
On Mon, 22 Apr 2024 at 11:44, jian he wrote:
>
> hi.
> minor issue in guc.c.
>
> set work_mem to '1kB';
> ERROR: 1 kB is outside the valid range for parameter "work_mem" (64
> .. 2147483647)
> should it be
> ERROR: 1 kB is outside the valid range for parameter "work_mem" (64
> kB .. 2147483647 kB)
> ?
> since the units for work_mem are { "B", "kB", "MB", "GB", and "TB"}
>
> search `outside the valid range for parameter`,
> there are two occurrences in guc.c.
Nice find. I agree it could cause confusion.
--
Regards,
Nazir Bilal Yavuz
Microsoft
Re: pg_combinebackup does not detect missing files
On Sun, Apr 21, 2024 at 8:47 PM David Steele wrote: > > I figured that wouldn't be particularly meaningful, and > > that's pretty much the only kind of validation that's even > > theoretically possible without a bunch of extra overhead, since we > > compute checksums on entire files rather than, say, individual blocks. > > And you could really only do it for the final backup in the chain, > > because you should end up accessing all of those files, but the same > > is not true for the predecessor backups. So it's a very weak form of > > verification. > > I don't think it is weak if you can verify that the output is exactly as > expected, i.e. all files are present and have the correct contents. I don't understand what you mean here. I thought we were in agreement that verifying contents would cost a lot more. The verification that we can actually do without much cost can only check for missing files in the most recent backup, which is quite weak. pg_verifybackup is available if you want more comprehensive verification and you're willing to pay the cost of it. > I tested the patch and it works, but there is some noise from WAL files > since they are not in the manifest: > > $ pg_combinebackup test/backup/full test/backup/incr1 -o test/backup/combine > pg_combinebackup: warning: "pg_wal/00010008" is present > on disk but not in the manifest > pg_combinebackup: error: "base/1/3596" is present in the manifest but > not on disk Oops, OK, that can be fixed. > I think it is a worthwhile change and we are still a month away from > beta1. We'll see if anyone disagrees. I don't plan to press forward with this in this release unless we get a couple of +1s from disinterested parties. We're now two weeks after feature freeze and this is design behavior, not a bug. Perhaps the design should have been otherwise, but two weeks after feature freeze is not the time to debate that. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Introduce XID age and inactive timeout based replication slot invalidation
Hi, On Thu, Apr 4, 2024 at 9:23 PM Bharath Rupireddy wrote: > > On Thu, Apr 4, 2024 at 4:35 PM Amit Kapila wrote: > > > > > Thanks for the changes. v34-0001 LGTM. > > > > I was doing a final review before pushing 0001 and found that > > 'inactive_since' could be set twice during startup after promotion, > > once while restoring slots and then via ShutDownSlotSync(). The reason > > is that ShutDownSlotSync() will be invoked in normal startup on > > primary though it won't do anything apart from setting inactive_since > > if we have synced slots. I think you need to check 'StandbyMode' in > > update_synced_slots_inactive_since() and return if the same is not > > set. We can't use 'InRecovery' flag as that will be set even during > > crash recovery. > > > > Can you please test this once unless you don't agree with the above theory? > > Nice catch. I've verified that update_synced_slots_inactive_since is > called even for normal server startups/crash recovery. I've added a > check to exit if the StandbyMode isn't set. > > Please find the attached v35 patch. > The documentation says about both 'active' and 'inactive_since' columns of pg_replication_slots say: --- active bool True if this slot is currently actively being used inactive_since timestamptz The time since the slot has become inactive. NULL if the slot is currently being used. Note that for slots on the standby that are being synced from a primary server (whose synced field is true), the inactive_since indicates the last synchronization (see Section 47.2.3) time. --- When reading the description I thought if 'active' is true, 'inactive_since' is NULL, but it doesn't seem to apply for temporary slots. Since we don't reset the active_pid field of temporary slots when the release, the 'active' is still true in the view but 'inactive_since' is not NULL. Do you think we need to mention it in the documentation? As for the timeout-based slot invalidation feature, we could end up invalidating the temporary slots even if they are shown as active, which could confuse users. Do we want to somehow deal with it? Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: GUC-ify walsender MAX_SEND_SIZE constant
Hey folks, Any news, comments, etc. about this thread? Best regards Majid Garoosi On Mon, 12 Feb 2024 at 01:10, Michael Paquier wrote: > On Sun, Feb 11, 2024 at 04:32:20PM +0330, Majid Garoosi wrote: > > On Fri, 9 Feb 2024 at 22:33, Andres Freund wrote: > >> The way we read the WAL data is perfectly prefetchable by the the OS, > so I > >> wouldn't really expect gains here. Have you actually been able to see a > >> performance benefit by increasing MAX_SEND_SIZE? > > > > Yes, I have seen a huge performance jump. Take a look at here > > < > https://www.postgresql.org/message-id/CAFWczPvi_5FWH%2BJTqkWbi%2Bw83hy%3DMYg%3D2hKK0%3DJZBe9%3DhTpE4w%40mail.gmail.com > > > > for > > more info. > > Yes, I can get the idea that grouping more replication messages in > one shot can be beneficial in some cases while being > environment-dependent, though I also get the point that we cannot > simply GUC-ify everything either. I'm OK with this one at the end, > because it is not performance critical. > > Note that it got lowered to the current value in ea5516081dcb to make > it more responsive, while being half a WAL segment in 40f908bdcdc7 > when WAL senders have been introduced in 2010. I cannot pinpoint the > exact thread that led to this change, but I'm adding Fujii-san and > Heikki in CC for comments. > -- > Michael >
Re: promotion related handling in pg_sync_replication_slots()
On Mon, Apr 22, 2024 at 9:02 PM shveta malik wrote:
>
> On Mon, Apr 22, 2024 at 5:10 PM Amit Kapila wrote:
> >
> > On Fri, Apr 19, 2024 at 1:52 PM shveta malik wrote:
> > >
> > > Please find v9 with the above comments addressed.
> > >
> >
> > I have made minor modifications in the comments and a function name.
> > Please see the attached top-up patch. Apart from this, the patch looks
> > good to me.
>
> Thanks for the patch, the changes look good Amit. Please find the merged
> patch.
>
I've reviewed the patch and have some comments:
---
/*
-* Early initialization.
+* Register slotsync_worker_onexit() before we register
+* ReplicationSlotShmemExit() in BaseInit(), to ensure that during the
+* exit of the slot sync worker, ReplicationSlotShmemExit() is called
+* first, followed by slotsync_worker_onexit(). The startup process during
+* promotion invokes ShutDownSlotSync() which waits for slot sync to
+* finish and it does that by checking the 'syncing' flag. Thus worker
+* must be done with the slots' release and cleanup before it marks itself
+* as finished syncing.
*/
I'm slightly worried that we register the slotsync_worker_onexit()
callback before BaseInit(), because it could be a blocker when we want
to add more work in the callback, for example sending the stats.
---
synchronize_slots(wrconn);
+
+ /* Cleanup the temporary slots */
+ ReplicationSlotCleanup();
+
+ /* We are done with sync, so reset sync flag */
+ reset_syncing_flag();
I think it ends up removing other temp slots that are created by the
same backend process using
pg_create_{physical,logical_replication_slots() function, which could
be a large side effect of this function for users. Also, if users want
to have a process periodically calling pg_sync_replication_slots()
instead of the slotsync worker, it doesn't support a case where we
create a temp not-ready slot and turn it into a persistent slot if
it's ready for sync.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Re: Support a wildcard in backtrace_functions
On Mon, Apr 22, 2024 at 2:36 AM Michael Paquier wrote: > I'd like to think about this stuff in a different way: this is useful > if enabled by default because it can also help in finding out error > paths that should not use the internal errcode. Normally, there > should be zero backtraces produced, except in unexpected > never-to-be-reached cases. That's long been my feeling about this. So, if we revert this for now, what we ought to do is put it back right ASAP after feature freeze and then clean all that up. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Support event trigger for logoff
On Sat, 20 Apr 2024 at 01:36, Tom Lane wrote: > Japin Li writes: >> I see [1] has already implemented on login event trigger, why not implement >> the logoff event trigger? > > What happens if the session crashes, or otherwise fails unexpectedly? > I am currently unsure how to handle such situations, but I believe it is not only for logoff events. >> Here is a problem with the regression test when using \c to create a new >> session, because it might be running concurrently, which may lead to the >> checking being unstable. > >> Any thoughts? > > Let's not go there at all. I don't think there's enough field > demand to justify dealing with this concept. > Thanks for your point out this for me. -- Regards, Japin Li
RE: Slow catchup of 2PC (twophase) transactions on replica in LR
> Dear Vitaly, > > > I looked at the patch and realized that I can't try it easily in the near > > future > > because the solution I'm working on is based on PG16 or earlier. This patch > > is > > not easily applicable to the older releases. I have to port my solution to > > the > > master, which is not done yet. > > We also tried to port our patch for PG16, but the largest barrier was that a > replication command ALTER_SLOT is not supported. Since the slot option > two_phase > can't be modified, it is difficult to skip decoding PREPARE command even when > altering the option from true to false. > IIUC, Adding a new feature (e.g., replication command) for minor updates is > generally > prohibited > > We must consider another approach for backpatching, but we do not have > solutions > for now. Attached patch set is a ported version for PG16, which breaks ABI. This can be used for testing purpose, but it won't be pushed to REL_16_STABLE. At least, this patchset can pass my github CI. Can you apply and check whether your issue is solved? Best Regards, Hayato Kuroda FUJITSU LIMITED https://www.fujitsu.com/ REL_16_0001-Allow-altering-of-two_phase-option-of-a-SUBSCRIPTION.patch Description: REL_16_0001-Allow-altering-of-two_phase-option-of-a-SUBSCRIPTION.patch REL_16_0002-Alter-slot-option-two_phase-only-when-altering-true-.patch Description: REL_16_0002-Alter-slot-option-two_phase-only-when-altering-true-.patch REL_16_0003-Abort-prepared-transactions-while-altering-two_phase.patch Description: REL_16_0003-Abort-prepared-transactions-while-altering-two_phase.patch REL_16_0004-Add-force_alter-option.patch Description: REL_16_0004-Add-force_alter-option.patch
Re: promotion related handling in pg_sync_replication_slots()
On Mon, Apr 22, 2024 at 5:10 PM Amit Kapila wrote: > > On Fri, Apr 19, 2024 at 1:52 PM shveta malik wrote: > > > > Please find v9 with the above comments addressed. > > > > I have made minor modifications in the comments and a function name. > Please see the attached top-up patch. Apart from this, the patch looks > good to me. Thanks for the patch, the changes look good Amit. Please find the merged patch. thanks Shveta v10-0001-Handle-stopSignaled-during-sync-function-call.patch Description: Binary data
Re: Avoid orphaned objects dependencies, take 3
22.04.2024 13:52, Bertrand Drouvot wrote:
That's weird, I just launched it several times with the patch applied and I'm
not
able to see the seg fault (while I can see it constently failing on the master
branch).
Are you 100% sure you tested it against a binary with the patch applied?
Yes, at least I can't see what I'm doing wrong. Please try my
self-contained script attached.
Best regards,
Alexander#!/bin/bash
git reset --hard HEAD; git clean -dfx >/dev/null
wget
https://www.postgresql.org/message-id/attachment/159685/v1-0001-Avoid-orphaned-objects-dependencies.patch
git apply v1-0001-Avoid-orphaned-objects-dependencies.patch || exit 1
CPPFLAGS="-Og" ./configure --enable-debug --enable-cassert -q && make -j8 -s &&
TESTS="test_setup" make -s check-tests
PGROOT=`pwd`/tmp_install
export PGDATA=`pwd`/tmpdb
export PATH="$PGROOT/usr/local/pgsql/bin:$PATH"
export LD_LIBRARY_PATH="$PGROOT/usr/local/pgsql/lib"
initdb >initdb.log 2>&1
export PGPORT=55432
echo "
port=$PGPORT
fsync = off
" > $PGDATA/postgresql.auto.conf
pg_ctl -l server.log start
export PGDATABASE=regression
createdb regression
for ((i=1;i<=300;i++)); do
( { for ((n=1;n<=20;n++)); do echo "DROP SCHEMA s;"; done } | psql )
>psql1.log 2>&1 &
echo "
CREATE SCHEMA s;
CREATE FUNCTION s.func1() RETURNS int LANGUAGE SQL AS 'SELECT 1;';
CREATE FUNCTION s.func2() RETURNS int LANGUAGE SQL AS 'SELECT 2;';
CREATE FUNCTION s.func3() RETURNS int LANGUAGE SQL AS 'SELECT 3;';
CREATE FUNCTION s.func4() RETURNS int LANGUAGE SQL AS 'SELECT 4;';
CREATE FUNCTION s.func5() RETURNS int LANGUAGE SQL AS 'SELECT 5;';
" | psql >psql2.log 2>&1 &
wait
psql -c "DROP SCHEMA s CASCADE" >psql3.log
done
echo "
SELECT pg_identify_object('pg_proc'::regclass, pp.oid, 0), pp.oid FROM pg_proc
pp
LEFT JOIN pg_namespace pn ON pp.pronamespace = pn.oid WHERE pn.oid IS NULL" |
psql
pg_ctl -w -t 5 -D "$PGDATA" stop
Re: promotion related handling in pg_sync_replication_slots()
On Fri, Apr 19, 2024 at 1:52 PM shveta malik wrote:
>
> Please find v9 with the above comments addressed.
>
I have made minor modifications in the comments and a function name.
Please see the attached top-up patch. Apart from this, the patch looks
good to me.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/replication/logical/slotsync.c
b/src/backend/replication/logical/slotsync.c
index 66fb46ac27..72a775b09c 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -79,10 +79,11 @@
* and also sets stopSignaled=true to handle the race condition when the
* postmaster has not noticed the promotion yet and thus may end up restarting
* the slot sync worker. If stopSignaled is set, the worker will exit in such a
- * case. Note that we don't need to reset this variable as after promotion the
- * slot sync worker won't be restarted because the pmState changes to PM_RUN
from
- * PM_HOT_STANDBY and we don't support demoting primary without restarting the
- * server. See MaybeStartSlotSyncWorker.
+ * case. The SQL function pg_sync_replication_slots() will also error out if
+ * this flag is set. Note that we don't need to reset this variable as after
+ * promotion the slot sync worker won't be restarted because the pmState
+ * changes to PM_RUN from PM_HOT_STANDBY and we don't support demoting
+ * primary without restarting the server. See MaybeStartSlotSyncWorker.
*
* The 'syncing' flag is needed to prevent concurrent slot syncs to avoid slot
* overwrites.
@@ -1246,12 +1247,11 @@ wait_for_slot_activity(bool some_slot_updated)
}
/*
- * Check stopSignaled and syncing flags. Emit error if promotion has
- * already set stopSignaled or if it is concurrent sync call. Otherwise,
- * set 'syncing' flag and pid info.
+ * Emit an error if a promotion or a concurrent sync call is in progress.
+ * Otherwise, advertise that a sync is in progress.
*/
static void
-check_flags_and_set_sync_info(pid_t worker_pid)
+check_and_set_sync_info(pid_t worker_pid)
{
SpinLockAcquire(>mutex);
@@ -1259,9 +1259,8 @@ check_flags_and_set_sync_info(pid_t worker_pid)
Assert(worker_pid == InvalidPid || SlotSyncCtx->pid == InvalidPid);
/*
-* Startup process signaled the slot sync machinery to stop, so if
-* meanwhile postmaster ended up starting the worker again or user has
-* invoked pg_sync_replication_slots(), error out.
+* Emit an error if startup process signaled the slot sync machinery to
+* stop. See comments atop SlotSyncCtxStruct.
*/
if (SlotSyncCtx->stopSignaled)
{
@@ -1281,7 +1280,10 @@ check_flags_and_set_sync_info(pid_t worker_pid)
SlotSyncCtx->syncing = true;
- /* Advertise our PID so that the startup process can kill us on
promotion */
+ /*
+* Advertise the required PID so that the startup process can kill the
slot
+* sync worker on promotion.
+*/
SlotSyncCtx->pid = worker_pid;
SpinLockRelease(>mutex);
@@ -1333,13 +1335,13 @@ ReplSlotSyncWorkerMain(char *startup_data, size_t
startup_data_len)
/*
* Register slotsync_worker_onexit() before we register
-* ReplicationSlotShmemExit() in BaseInit(), to ensure that during exit
of
-* slot sync worker, ReplicationSlotShmemExit() is called first,
followed
-* by slotsync_worker_onexit(). Startup process during promotion calls
-* ShutDownSlotSync() which waits for slot sync to finish and it does
that
-* by checking the 'syncing' flag. Thus it is important that worker
should
-* be done with slots' release and cleanup before it actually marks
itself
-* as finished syncing.
+* ReplicationSlotShmemExit() in BaseInit(), to ensure that during the
exit
+* of the slot sync worker, ReplicationSlotShmemExit() is called first,
+* followed by slotsync_worker_onexit(). The startup process during
+* promotion invokes ShutDownSlotSync() which waits for slot sync to
finish
+* and it does that by checking the 'syncing' flag. Thus worker must be
+* done with the slots' release and cleanup before it marks itself as
+* finished syncing.
*/
before_shmem_exit(slotsync_worker_onexit, (Datum) 0);
@@ -1391,7 +1393,7 @@ ReplSlotSyncWorkerMain(char *startup_data, size_t
startup_data_len)
pqsignal(SIGPIPE, SIG_IGN);
pqsignal(SIGCHLD, SIG_DFL);
- check_flags_and_set_sync_info(MyProcPid);
+ check_and_set_sync_info(MyProcPid);
ereport(LOG, errmsg("slot sync worker started"));
@@ -1544,9 +1546,9 @@ update_synced_slots_inactive_since(void)
/*
* Shut down the slot sync worker.
*
- * It sends signal to shutdown slot sync worker. It also waits till
- * the slot sync worker has exited and pg_sync_replication_slots()
- * has finished.
+ * This function sends signal to shutdown slot sync
Re: Avoid orphaned objects dependencies, take 3
Hi,
On Mon, Apr 22, 2024 at 01:00:00PM +0300, Alexander Lakhin wrote:
> Hi Bertrand,
>
> 22.04.2024 11:45, Bertrand Drouvot wrote:
> > Hi,
> >
> > This new thread is a follow-up of [1] and [2].
> >
> > Problem description:
> >
> > We have occasionally observed objects having an orphaned dependency, the
> > most common case we have seen is functions not linked to any namespaces.
> >
> > ...
> >
> > Looking forward to your feedback,
>
> This have reminded me of bug #17182 [1].
Thanks for the link to the bug!
> Unfortunately, with the patch applied, the following script:
>
> for ((i=1;i<=100;i++)); do
> ( { for ((n=1;n<=20;n++)); do echo "DROP SCHEMA s;"; done } | psql )
> >psql1.log 2>&1 &
> echo "
> CREATE SCHEMA s;
> CREATE FUNCTION s.func1() RETURNS int LANGUAGE SQL AS 'SELECT 1;';
> CREATE FUNCTION s.func2() RETURNS int LANGUAGE SQL AS 'SELECT 2;';
> CREATE FUNCTION s.func3() RETURNS int LANGUAGE SQL AS 'SELECT 3;';
> CREATE FUNCTION s.func4() RETURNS int LANGUAGE SQL AS 'SELECT 4;';
> CREATE FUNCTION s.func5() RETURNS int LANGUAGE SQL AS 'SELECT 5;';
> " | psql >psql2.log 2>&1 &
> wait
> psql -c "DROP SCHEMA s CASCADE" >psql3.log
> done
> echo "
> SELECT pg_identify_object('pg_proc'::regclass, pp.oid, 0), pp.oid FROM
> pg_proc pp
> LEFT JOIN pg_namespace pn ON pp.pronamespace = pn.oid WHERE pn.oid IS NULL"
> | psql
>
> still ends with:
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
>
> 2024-04-22 09:54:39.171 UTC|||662633dc.152bbc|LOG: server process (PID
> 1388378) was terminated by signal 11: Segmentation fault
> 2024-04-22 09:54:39.171 UTC|||662633dc.152bbc|DETAIL: Failed process was
> running: SELECT pg_identify_object('pg_proc'::regclass, pp.oid, 0), pp.oid
> FROM pg_proc pp
> LEFT JOIN pg_namespace pn ON pp.pronamespace = pn.oid WHERE pn.oid IS
> NULL
>
Thanks for sharing the script.
That's weird, I just launched it several times with the patch applied and I'm
not
able to see the seg fault (while I can see it constently failing on the master
branch).
Are you 100% sure you tested it against a binary with the patch applied?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Re: cataloguing NOT NULL constraints
Hi Alexander,
On 2024-Apr-18, Alexander Lakhin wrote:
> 18.04.2024 16:39, Alvaro Herrera wrote:
> > I have pushed a fix which should hopefully fix this problem
> > (d9f686a72e). Please give this a look. Thanks for reporting the issue.
>
> Please look at an assertion failure, introduced with d9f686a72:
> CREATE TABLE t(a int, NOT NULL a NO INHERIT);
> CREATE TABLE t2() INHERITS (t);
>
> ALTER TABLE t ADD CONSTRAINT nna NOT NULL a;
> TRAP: failed Assert("lockmode != NoLock || IsBootstrapProcessingMode() ||
> CheckRelationLockedByMe(r, AccessShareLock, true)"), File: "relation.c",
> Line: 67, PID: 2980258
Ah, of course -- we're missing acquiring locks during the prep phase for
the recursive case of ADD CONSTRAINT. So we just need to add
find_all_inheritors() to do so in the AT_AddConstraint case in
ATPrepCmd(). However these naked find_all_inheritors() call look a bit
ugly to me, so I couldn't resist the temptation of adding a static
function ATLockAllDescendants to clean it up a bit. I'll also add your
script to the tests and push shortly.
> On d9f686a72~1 this script results in:
> ERROR: cannot change NO INHERIT status of inherited NOT NULL constraint
> "t_a_not_null" on relation "t"
Right. Now I'm beginning to wonder if allowing ADD CONSTRAINT to mutate
a pre-existing NO INHERIT constraint into a inheritable constraint
(while accepting a constraint name in the command that we don't heed) is
really what we want. Maybe we should throw some error when the affected
constraint is the topmost one, and only accept the inheritance status
change when we're recursing.
Also I just noticed that in 9b581c534186 (which introduced this error
message) I used ERRCODE_DATATYPE_MISMATCH ... Is that really appropriate
here?
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Once again, thank you and all of the developers for your hard work on
PostgreSQL. This is by far the most pleasant management experience of
any database I've worked on." (Dan Harris)
http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php
>From 35b485b72d0675d631cde9f95e65d9c0db9254b8 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Mon, 22 Apr 2024 11:32:04 +0200
Subject: [PATCH] Acquire locks on children before recursing
ALTER TABLE ADD CONSTRAINT was missing this, as evidenced by assertion
failures.
Reported-by: Alexander Lakhin
Discussion: https://postgr.es/m/5b4cd32f-1d5b-c080-c688-31736bbcd...@gmail.com
---
src/backend/commands/tablecmds.c | 33 ++--
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3556240c8e..8941181912 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -427,6 +427,7 @@ static void ATSimplePermissions(AlterTableType cmdtype, Relation rel, int allowe
static void ATSimpleRecursion(List **wqueue, Relation rel,
AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode,
AlterTableUtilityContext *context);
+static void ATLockAllDescendants(Oid relid, LOCKMODE lockmode);
static void ATCheckPartitionsNotInUse(Relation rel, LOCKMODE lockmode);
static void ATTypedTableRecursion(List **wqueue, Relation rel, AlterTableCmd *cmd,
LOCKMODE lockmode,
@@ -1621,9 +1622,7 @@ RemoveRelations(DropStmt *drop)
* will lock those objects in the other order.
*/
if (state.actual_relkind == RELKIND_PARTITIONED_INDEX)
- (void) find_all_inheritors(state.heapOid,
- state.heap_lockmode,
- NULL);
+ ATLockAllDescendants(state.heapOid, state.heap_lockmode);
/* OK, we're ready to delete this one */
obj.classId = RelationRelationId;
@@ -4979,10 +4978,15 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_AddConstraint: /* ADD CONSTRAINT */
ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
- /* Recursion occurs during execution phase */
- /* No command-specific prep needed except saving recurse flag */
if (recurse)
+ {
+/*
+ * Make note for execution phase about need for recursion;
+ * also acquire lock on all descendants.
+ */
+ATLockAllDescendants(RelationGetRelid(rel), lockmode);
cmd->recurse = true;
+ }
pass = AT_PASS_ADD_CONSTR;
break;
case AT_AddIndexConstraint: /* ADD CONSTRAINT USING INDEX */
@@ -6721,6 +6725,17 @@ ATSimpleRecursion(List **wqueue, Relation rel,
}
}
+/*
+ * ATLockAllDescendants
+ *
+ * Acquire lock on all descendants of the given relation.
+ */
+static void
+ATLockAllDescendants(Oid relid, LOCKMODE lockmode)
+{
+ (void) find_all_inheritors(relid, lockmode, NULL);
+}
+
/*
* Obtain list of partitions of the given table, locking them all at the given
* lockmode and ensuring that they all pass CheckTableNotInUse.
@@ -9370,10 +9385,9 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd,
/*
*
Re: AIX support
Hi Team, > I have some sympathy for this. The discussion about removing AIX > support had a very short turnaround and happened in an unrelated thread, > without any sort of public announcement or consultation. So this report > of "hey, we were still using that" is timely and fair. We would really like to thank you & the team for considering our request, and really appreciate for providing all the possible options to support AIX. > But the underlying issue that led to the removal (something to do with > direct I/O support and alignment) would still need to be addressed. As we already validated that these alignment specific issues are resolved with the latest versions of the compilers (gcc/ibm-clang). We would request you to use the latest versions for the build. > If we are making the reinstatement of AIX > support contingent on new buildfarm support, those machines need to be > available, at least initially, at least for back branches, like in a > week. Which seems tight. We are already working with the internal team in procuring the nodes for the buildfarm, which can be accessible by the community. > I can see several ways going forward: > 1. We revert the removal of AIX support and carry on with the status quo > ante. (The removal of AIX is a regression; it is timely and in scope > now to revert the change.) > 2. Like (1), but we consider that notice has been given, and we will > remove it early in PG18 (like August) unless the situation improves. We would really appreciate you for providing the possible options and we are very much inclined to these above approaches. Regards, Sriram.
Re: Avoid orphaned objects dependencies, take 3
Hi Bertrand,
22.04.2024 11:45, Bertrand Drouvot wrote:
Hi,
This new thread is a follow-up of [1] and [2].
Problem description:
We have occasionally observed objects having an orphaned dependency, the
most common case we have seen is functions not linked to any namespaces.
...
Looking forward to your feedback,
This have reminded me of bug #17182 [1].
Unfortunately, with the patch applied, the following script:
for ((i=1;i<=100;i++)); do
( { for ((n=1;n<=20;n++)); do echo "DROP SCHEMA s;"; done } | psql ) >psql1.log
2>&1 &
echo "
CREATE SCHEMA s;
CREATE FUNCTION s.func1() RETURNS int LANGUAGE SQL AS 'SELECT 1;';
CREATE FUNCTION s.func2() RETURNS int LANGUAGE SQL AS 'SELECT 2;';
CREATE FUNCTION s.func3() RETURNS int LANGUAGE SQL AS 'SELECT 3;';
CREATE FUNCTION s.func4() RETURNS int LANGUAGE SQL AS 'SELECT 4;';
CREATE FUNCTION s.func5() RETURNS int LANGUAGE SQL AS 'SELECT 5;';
" | psql >psql2.log 2>&1 &
wait
psql -c "DROP SCHEMA s CASCADE" >psql3.log
done
echo "
SELECT pg_identify_object('pg_proc'::regclass, pp.oid, 0), pp.oid FROM pg_proc
pp
LEFT JOIN pg_namespace pn ON pp.pronamespace = pn.oid WHERE pn.oid IS NULL" |
psql
still ends with:
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
2024-04-22 09:54:39.171 UTC|||662633dc.152bbc|LOG: server process (PID 1388378) was terminated by signal 11:
Segmentation fault
2024-04-22 09:54:39.171 UTC|||662633dc.152bbc|DETAIL: Failed process was running: SELECT
pg_identify_object('pg_proc'::regclass, pp.oid, 0), pp.oid FROM pg_proc pp
LEFT JOIN pg_namespace pn ON pp.pronamespace = pn.oid WHERE pn.oid IS NULL
[1]
https://www.postgresql.org/message-id/flat/17182-a6baa001dd1784be%40postgresql.org
Best regards,
Alexander
Re: Disallow changing slot's failover option in transaction block
Hi, On Mon, Apr 22, 2024 at 11:32:14AM +0530, shveta malik wrote: > On Mon, Apr 22, 2024 at 5:57 AM Zhijie Hou (Fujitsu) > wrote: > > Attach the V3 patch which also addressed Shveta[1] and Bertrand[2]'s > > comments. Thanks! > Tested the patch, works well. Same here. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
RE: Slow catchup of 2PC (twophase) transactions on replica in LR
Dear hackers, > Looking at this a bit more, maybe rolling back all prepared transactions on > the > subscriber when toggling two_phase from true to false might not be desirable > for the customer. Maybe we should have an option for customers to control > whether transactions should be rolled back or not. Maybe transactions should > only be rolled back if a "force" option is also set. What do people think? And here is a patch for adds new option "force_alter" (better name is very welcome). It could be used only when altering two_phase option. Let me share examples. Assuming that there are logical replication system with two_phase = on, and there are prepared transactions: ``` subscriber=# SELECT * FROM pg_prepared_xacts ; transaction | gid| prepared| owner | database -+--+---+--+-- 741 | pg_gid_16390_741 | 2024-04-22 08:02:34.727913+00 | postgres | postgres 742 | pg_gid_16390_742 | 2024-04-22 08:02:34.729486+00 | postgres | postgres (2 rows) ``` At that time, altering two_phase to false alone will be failed: ``` subscriber=# ALTER SUBSCRIPTION sub DISABLE ; ALTER SUBSCRIPTION subscriber=# ALTER SUBSCRIPTION sub SET (two_phase = off); ERROR: cannot alter two_phase = false when there are prepared transactions ``` It succeeds if force_alter is also expressly set. Prepared transactions will be aborted at that time. ``` subscriber=# ALTER SUBSCRIPTION sub SET (two_phase = off, force_alter = on); ALTER SUBSCRIPTION subscriber=# SELECT * FROM pg_prepared_xacts ; transaction | gid | prepared | owner | database -+-+--+---+-- (0 rows) ``` Best Regards, Hayato Kuroda FUJITSU LIMITED https://www.fujitsu.com/global/ v5-0001-Allow-altering-of-two_phase-option-of-a-SUBSCRIPT.patch Description: v5-0001-Allow-altering-of-two_phase-option-of-a-SUBSCRIPT.patch v5-0002-Alter-slot-option-two_phase-only-when-altering-tr.patch Description: v5-0002-Alter-slot-option-two_phase-only-when-altering-tr.patch v5-0003-Abort-prepared-transactions-while-altering-two_ph.patch Description: v5-0003-Abort-prepared-transactions-while-altering-two_ph.patch v5-0004-Add-force_alter-option.patch Description: v5-0004-Add-force_alter-option.patch
Re: POC: GROUP BY optimization
On Fri, Apr 19, 2024 at 6:44 PM jian he wrote: > > On Thu, Apr 18, 2024 at 6:58 PM Alexander Korotkov > wrote: > > > > Thank you for the fixes you've proposed. I didn't look much into > > details yet, but I think the main concern Tom expressed in [1] is > > whether the feature is reasonable at all. I think at this stage the > > most important thing is to come up with convincing examples showing > > how huge performance benefits it could cause. I will return to this > > later today and will try to provide some convincing examples. > > hi. previously preprocess_groupclause will not process cases where no ORDER BY clause is specified. commit 0452b461b will reorder the GROUP BY element even though no ORDER BY clause is specified , if there are associated indexes on it. (hope I understand it correctly). for example (when enable_hashagg is false) explain(verbose) select count(*) FROM btg GROUP BY y,x; in pg16 will not reorder, it will be as is: `GROUP BY y,x` after commit 0452b461b, it will reorder to `GROUP BY x,y`. because there is an index `btree (x, y)` (only one) associated with it. if you drop the index `btree (x, y)` , it will be `GROUP BY y,x` as pg16. This reordering GROUP BY element when no ORDER BY clause is not specified is performant useful when the work_mem is small. I've attached some tests comparing master with REL_16_STABLE to demonstrate that. all the tests attached are under the condition: work_mem='64kB', buildtype=release, max_parallel_workers_per_gather=0. one example: CREATE TABLE btg5 AS SELECT i::numeric % 10 AS x, i % 10 AS y, 'abc' || i % 10 AS z, i % 10 AS w FROM generate_series(1, 1e6) AS i; CREATE INDEX btg5_x_y_idx ON btg5(x, y); explain(analyze) SELECT count(*) FROM btg5 GROUP BY z, y, w, x; in pg17, the execution time is: 746.574 ms in pg16, the execution time is: 1693.483 ms if I reorder it manually as: `explain(analyze) SELECT count(*) FROM btg5 GROUP BY x, y, w, z;` then in pg16, the execution time is 630.394 ms low_mem_groupby_reorder.sql Description: application/sql
RE: Slow catchup of 2PC (twophase) transactions on replica in LR
Dear Vitaly, > I looked at the patch and realized that I can't try it easily in the near > future > because the solution I'm working on is based on PG16 or earlier. This patch is > not easily applicable to the older releases. I have to port my solution to the > master, which is not done yet. We also tried to port our patch for PG16, but the largest barrier was that a replication command ALTER_SLOT is not supported. Since the slot option two_phase can't be modified, it is difficult to skip decoding PREPARE command even when altering the option from true to false. IIUC, Adding a new feature (e.g., replication command) for minor updates is generally prohibited We must consider another approach for backpatching, but we do not have solutions for now. Best Regards, Hayato Kuroda FUJITSU LIMITED https://www.fujitsu.com/global/
Re: Patch to avoid orphaned dependencies
Hi, On Wed, Mar 23, 2022 at 12:49:06PM -0400, Tom Lane wrote: > Realistically, if we want to prevent this type of problem, then all > creation DDL will have to take a lock on each referenced object that'd > conflict with a lock taken by DROP. This might not be out of reach: > I think we do already take such locks while dropping objects. The > reference-side lock could be taken by the recordDependency mechanism > itself, ensuring that we don't miss anything; and that would also > allow us to not bother taking such a lock on pinned objects, which'd > greatly cut the cost (though not to zero). Thanks for the idea (and sorry for the delay replying to it)! I had a look at it and just created a new thread [1] based on your proposal. [1]: https://www.postgresql.org/message-id/flat/ZiYjn0eVc7pxVY45%40ip-10-97-1-34.eu-west-3.compute.internal Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Avoid orphaned objects dependencies, take 3
Hi, This new thread is a follow-up of [1] and [2]. Problem description: We have occasionally observed objects having an orphaned dependency, the most common case we have seen is functions not linked to any namespaces. Examples to produce such orphaned dependencies: Scenario 1: session 1: begin; drop schema schem; session 2: create a function in the schema schem session 1: commit; With the above, the function created in session 2 would be linked to a non existing schema. Scenario 2: session 1: begin; create a function in the schema schem session 2: drop schema schem; session 1: commit; With the above, the function created in session 1 would be linked to a non existing schema. A patch has been initially proposed to fix this particular (function-to-namespace) dependency (see [1]), but there could be much more scenarios (like the function-to-datatype one highlighted by Gilles in [1] that could lead to a function having an invalid parameter datatype). As Tom said there are dozens more cases that would need to be considered, and a global approach to avoid those race conditions should be considered instead. A first global approach attempt has been proposed in [2] making use of a dirty snapshot when recording the dependency. But this approach appeared to be "scary" and it was still failing to close some race conditions (see [2] for details). Then, Tom proposed another approach in [2] which is that "creation DDL will have to take a lock on each referenced object that'd conflict with a lock taken by DROP". This is what the attached patch is trying to achieve. It does the following: 1) A new lock (that conflicts with a lock taken by DROP) has been put in place when the dependencies are being recorded. Thanks to it, the drop schema in scenario 2 would be locked (resulting in an error should session 1 committs). 2) After locking the object while recording the dependency, the patch checks that the object still exists. Thanks to it, session 2 in scenario 1 would be locked and would report an error once session 1 committs (that would not be the case should session 1 abort the transaction). The patch also adds a few tests for some dependency cases (that would currently produce orphaned objects): - schema and function (as the above scenarios) - function and type - table and type (which is I think problematic enough, as involving a table into the game, to fix this stuff as a whole). [1]: https://www.postgresql.org/message-id/flat/a4f55089-7cbd-fe5d-a9bb-19adc6418...@darold.net#9af5cdaa9e80879beb1def3604c976e8 [2]: https://www.postgresql.org/message-id/flat/8369ff70-0e31-f194-2954-787f4d9e21dd%40amazon.com Please note that I'm not used to with this area of the code so that the patch might not take the option proposed by Tom the "right" way. Adding the patch to the July CF. Looking forward to your feedback, Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com >From 6798e85b0679dfccfa665007b06d9f1a0e39e0c6 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot Date: Fri, 29 Mar 2024 15:43:26 + Subject: [PATCH v1] Avoid orphaned objects dependencies It's currently possible to create orphaned objects dependencies, for example: Scenario 1: session 1: begin; drop schema schem; session 2: create a function in the schema schem session 1: commit; With the above, the function created in session 2 would be linked to a non existing schema. Scenario 2: session 1: begin; create a function in the schema schem session 2: drop schema schem; session 1: commit; With the above, the function created in session 1 would be linked to a non existing schema. To avoid those scenarios, a new lock (that conflicts with a lock taken by DROP) has been put in place when the dependencies are being recorded. With this in place, the drop schema in scenario 2 would be locked. Also, after locking the object, the patch checks that the object still exists: with this in place session 2 in scenario 1 would be locked and would report an error once session 1 committs (that would not be the case should session 1 abort the transaction). The patch adds a few tests for some dependency cases (that would currently produce orphaned objects): - schema and function (as the above scenarios) - function and type - table and type --- src/backend/catalog/dependency.c | 42 ++ src/backend/catalog/objectaddress.c | 57 +++ src/backend/catalog/pg_depend.c | 6 ++ src/include/catalog/dependency.h | 1 + src/include/catalog/objectaddress.h | 1 + src/test/modules/Makefile | 1 + src/test/modules/meson.build | 1 + .../test_dependencies_locks/.gitignore| 3 + .../modules/test_dependencies_locks/Makefile | 14 + .../expected/test_dependencies_locks.out | 49 .../test_dependencies_locks/meson.build |
slightly misleading Error message in guc.c
hi.
minor issue in guc.c.
set work_mem to '1kB';
ERROR: 1 kB is outside the valid range for parameter "work_mem" (64
.. 2147483647)
should it be
ERROR: 1 kB is outside the valid range for parameter "work_mem" (64
kB .. 2147483647 kB)
?
since the units for work_mem are { "B", "kB", "MB", "GB", and "TB"}
search `outside the valid range for parameter`,
there are two occurrences in guc.c.
Re: Fix parallel vacuum buffer usage reporting
On Sat, Apr 20, 2024 at 2:00 PM Alena Rybakina
wrote:
> Hi, thank you for your work with this subject.
>
> While I was reviewing your code, I noticed that your patch conflicts with
> another patch [0] that been committed.
>
> [0]
> https://www.postgresql.org/message-id/flat/CA%2BhUKGJkOiOCa%2Bmag4BF%2BzHo7qo%3Do9CFheB8%3Dg6uT5TUm2gkvA%40mail.gmail.com
>
I've rebased the patch and also split the changes:
1: Use pgBufferUsage in Vacuum and Analyze block reporting
2: Stop tracking buffer usage in the now unused Vacuum{Hit,Miss,Dirty}
variables
3: Remove declarations of Vacuum{Hit,Miss,Dirty}
v3-0003-Remove-declaration-of-Vacuum-block-usage-tracking.patch
Description: Binary data
v3-0001-Fix-parallel-vacuum-buffer-usage-reporting.patch
Description: Binary data
v3-0002-Remove-unused-tracking-of-vacuum-buffer-usage.patch
Description: Binary data
Re: Optimizing nbtree ScalarArrayOp execution, allowing multi-column ordered scans, skip scan
Hello Peter,
07.04.2024 20:18, Peter Geoghegan wrote:
On Sun, Apr 7, 2024 at 1:00 PM Alexander Lakhin wrote:
SELECT * FROM t WHERE a < ANY (ARRAY[1]) AND b < ANY (ARRAY[1]);
TRAP: failed Assert("so->numArrayKeys"), File: "nbtutils.c", Line: 560, PID:
3251267
I immediately see what's up here. WIll fix this in the next short
while. There is no bug here in builds without assertions, but it's
probably best to keep the assertion, and to just make sure not to call
_bt_preprocess_array_keys_final() unless it has real work to do.
Please look at another case, where a similar Assert (but this time in
_bt_preprocess_keys()) is triggered:
CREATE TABLE t (a text, b text);
INSERT INTO t (a, b) SELECT 'a', repeat('b', 100) FROM generate_series(1, 500)
g;
CREATE INDEX t_idx ON t USING btree(a);
BEGIN;
DECLARE c CURSOR FOR SELECT a FROM t WHERE a = 'a';
FETCH FROM c;
FETCH RELATIVE 0 FROM c;
TRAP: failed Assert("so->numArrayKeys"), File: "nbtutils.c", Line: 2582, PID:
1130962
Best regards,
Alexander
Re: [PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS
On Mon, 22 Apr 2024 at 06:04, Michael Paquier wrote: > > On Fri, Apr 19, 2024 at 12:47:43PM +0300, Aleksander Alekseev wrote: > > Thanks. I see a few pieces of code that use special FOO_NUMBER enum > > values instead of a macro. Should we refactor these pieces > > accordingly? PFA another patch. > > I don't see why not for the places you are changing here, we can be > more consistent. [Shrug] I do prefer using a macro. Adding a counter element to the end of the enum feels like a hack, because the counter isn't the same kind of thing as all the other enum elements, so it feels out of place in the enum. On the other hand, I think it's a fairly common pattern that most people will recognise, and for other enums that are more likely to grow over time, it might be less error-prone than a macro, which people might overlook and fail to update. > Now, such changes are material for v18. Agreed. This has been added to the next commitfest, so let's see what others think. Regards, Dean
Re: Direct SSL connection with ALPN and HBA rules
On 22/04/2024 10:19, Michael Paquier wrote: On Sat, Apr 20, 2024 at 12:43:24AM +0300, Heikki Linnakangas wrote: On 19/04/2024 19:48, Jacob Champion wrote: On Fri, Apr 19, 2024 at 6:56 AM Heikki Linnakangas wrote: With direct SSL negotiation, we always require ALPN. (As an aside: I haven't gotten to test the version of the patch that made it into 17 yet, but from a quick glance it looks like we're not rejecting mismatched ALPN during the handshake as noted in [1].) Ah, good catch, that fell through the cracks. Agreed, the client should reject a direct SSL connection if the server didn't send ALPN. I'll add that to the Open Items so we don't forget again. Would somebody like to write a patch for that? I'm planning to look at this code more closely, as well. I plan to write the patch later today. Controlling these in HBA is a bit inconvenient, because you only find out after authentication if it's allowed or not. So if e.g. direct SSL connections are disabled for a user, Hopefully disabling direct SSL piecemeal is not a desired use case? I'm not sure it makes sense to focus on that. Forcing it to be enabled shouldn't have the same problem, should it? I'd get behind the case where a server rejects everything except direct SSL, yeah. Sticking that into a format similar to HBA rules would easily give the flexibility to be able to accept or reject direct or default SSL, though, while making it easy to parse. The implementation is not really complicated, and not far from the existing hostssl and nohostssl. As a whole, I can get behind a unique GUC that forces the use of direct. Or, we could extend the existing "ssl" GUC with a new "direct" value to accept only direct connections and restrict the original protocol (and a new "postgres" for the pre-16 protocol, rejecting direct?), while "on" is able to accept both. I'd be OK with that, although I still don't really see the point of forcing this from the server side. We could also add this later. Forcing it to be enabled piecemeal based on role or database has similar problems. Forcing it enabled for all connections seems sensible, though. Forcing it enabled based on the client's source IP address, but not user/database would be somewhat sensible too, but we don't currently have the HBA code to check the source IP and accept/reject SSLRequest based on that. The HBA rejection always happens after the client has sent the startup packet. Hmm. Splitting the logic checking HBA entries (around check_hba) so as we'd check for a portion of its contents depending on what the server has received or not from the client would not be that complicated. I'd question whether it makes sense to mix this information within the same configuration files as the ones holding the current HBA rules. If the same rules are used for the pre-startup-packet phase and the post-startup-packet phase, we'd want new keywords for these HBA rules, something different than the existing sslmode and no sslmode? Sounds complicated, and I don't really see the use case for controlling the direct SSL support in such a fine-grained fashion. It would be nice if we could reject non-SSL connections before the client sends the startup packet, but that's not possible because in a plaintext connection, that's the first packet that the client sends. The reverse would be possible: reject SSLRequest or direct SSL connection immediately, if HBA doesn't allow non-SSL connections from that IP address. But that's not very interesting. HBA-based control would certainly be v18 material. -- Heikki Linnakangas Neon (https://neon.tech)
Re: Okay to remove mention of mystery @ and ~ operators?
Thanks all! On Fri, 19 Apr 2024 at 13:59, Daniel Gustafsson wrote: > > On 19 Apr 2024, at 13:49, Daniel Gustafsson wrote: > >> On 19 Apr 2024, at 12:31, Aleksander Alekseev > wrote: > > >> Here is the patch. > > > > Nice catch, and thanks for the patch. I'll apply it with a backpatch to > when > > they were removed. > > Done, thanks for the report and the patch! > > -- > Daniel Gustafsson > >
Re: Slow catchup of 2PC (twophase) transactions on replica in LR
On Thu, Apr 18, 2024 at 4:26 PM Ajin Cherian wrote: > > Attaching the patch for your review and comments. Big thanks to Kuroda-san > for also working on the patch. > > Looking at this a bit more, maybe rolling back all prepared transactions on the subscriber when toggling two_phase from true to false might not be desirable for the customer. Maybe we should have an option for customers to control whether transactions should be rolled back or not. Maybe transactions should only be rolled back if a "force" option is also set. What do people think? regards, Ajin Cherian Fujitsu Australia
Re: Direct SSL connection with ALPN and HBA rules
On Sat, Apr 20, 2024 at 12:43:24AM +0300, Heikki Linnakangas wrote: > On 19/04/2024 19:48, Jacob Champion wrote: >> On Fri, Apr 19, 2024 at 6:56 AM Heikki Linnakangas wrote: >>> With direct SSL negotiation, we always require ALPN. >> >> (As an aside: I haven't gotten to test the version of the patch that >> made it into 17 yet, but from a quick glance it looks like we're not >> rejecting mismatched ALPN during the handshake as noted in [1].) > > Ah, good catch, that fell through the cracks. Agreed, the client should > reject a direct SSL connection if the server didn't send ALPN. I'll add that > to the Open Items so we don't forget again. Would somebody like to write a patch for that? I'm planning to look at this code more closely, as well. >> You get protection against attacks that could have otherwise happened >> during the plaintext portion of the handshake. That has architectural >> implications for more advanced uses of SCRAM, and it should prevent >> any repeats of CVE-2021-23222/23214. And if the peer doesn't pass the >> TLS handshake, they can't send you anything that you might forget is >> untrusted (like, say, an error message). > > Can you elaborate on the more advanced uses of SCRAM? I'm not sure what you mean here, either, Jacob. >>> Controlling these in HBA is a bit inconvenient, because you only find >>> out after authentication if it's allowed or not. So if e.g. direct SSL >>> connections are disabled for a user, >> >> Hopefully disabling direct SSL piecemeal is not a desired use case? >> I'm not sure it makes sense to focus on that. Forcing it to be enabled >> shouldn't have the same problem, should it? I'd get behind the case where a server rejects everything except direct SSL, yeah. Sticking that into a format similar to HBA rules would easily give the flexibility to be able to accept or reject direct or default SSL, though, while making it easy to parse. The implementation is not really complicated, and not far from the existing hostssl and nohostssl. As a whole, I can get behind a unique GUC that forces the use of direct. Or, we could extend the existing "ssl" GUC with a new "direct" value to accept only direct connections and restrict the original protocol (and a new "postgres" for the pre-16 protocol, rejecting direct?), while "on" is able to accept both. > Forcing it to be enabled piecemeal based on role or database has similar > problems. Forcing it enabled for all connections seems sensible, though. > Forcing it enabled based on the client's source IP address, but not > user/database would be somewhat sensible too, but we don't currently have > the HBA code to check the source IP and accept/reject SSLRequest based on > that. The HBA rejection always happens after the client has sent the startup > packet. Hmm. Splitting the logic checking HBA entries (around check_hba) so as we'd check for a portion of its contents depending on what the server has received or not from the client would not be that complicated. I'd question whether it makes sense to mix this information within the same configuration files as the ones holding the current HBA rules. If the same rules are used for the pre-startup-packet phase and the post-startup-packet phase, we'd want new keywords for these HBA rules, something different than the existing sslmode and no sslmode? -- Michael signature.asc Description: PGP signature
Re: Support a wildcard in backtrace_functions
On Sun, Apr 21, 2024 at 09:26:38AM +0200, Peter Eisentraut wrote: > Note that a standard test run produces a number of internal errors. I > haven't checked how likely these are in production, but one might want to > consider that before starting to dump backtraces in routine situations. > > For example, > > $ PG_TEST_INITDB_EXTRA_OPTS='-c backtrace_on_internal_error=on' meson test > -C build > $ grep -r 'BACKTRACE:' build/testrun | wc -l > 85 Ugh, I would not have expected that much. Isn't the problem you are reporting here entirely unrelated, though? It seems to me that these error paths should be using a proper errcode rather than the internal errcode, as these refer to states that can be reached by the user with a combination of queries and/or cancellations. For example, take this one for the REFRESH matview path which is a valid error, still using an elog(): if (!foundUniqueIndex) elog(ERROR, "could not find suitable unique index on materialized view"); I'd like to think about this stuff in a different way: this is useful if enabled by default because it can also help in finding out error paths that should not use the internal errcode. Normally, there should be zero backtraces produced, except in unexpected never-to-be-reached cases. -- Michael signature.asc Description: PGP signature
Re: Assert failure in _bt_preprocess_array_keys
On Mon, Apr 22, 2024 at 10:52 AM Peter Geoghegan wrote: > On Sun, Apr 21, 2024 at 10:36 PM Richard Guo > wrote: > > I didn't spend much time digging into it, but I wonder if this Assert is > > sensible. I noticed that before commit 5bf748b86b, the two datatypes > > were not equal to each other either (anyrange vs. int4range). > > The assertion is wrong. It is testing behavior that's much older than > commit 5bf748b86b, though. We can just get rid of it, since all of the > information that we'll actually apply when preprocessing scan keys > comes from the operator class. > > Pushed a fix removing the assertion just now. Thanks for the report. That's so quick. Thank you for the prompt fix. Thanks Richard
Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
On Sat, Apr 20, 2024 at 11:55:46AM +0900, Michael Paquier wrote: > FYI, as this is an open item, I am planning to wrap that at the > beginning of next week after a second lookup. If there are any > comments, feel free. This one is now fixed with f46bee346c3b, and the open item is marked as fixed. -- Michael signature.asc Description: PGP signature
Re: Disallow changing slot's failover option in transaction block
On Mon, Apr 22, 2024 at 5:57 AM Zhijie Hou (Fujitsu) wrote: > > On Friday, April 19, 2024 10:54 AM Kuroda, Hayato/黒田 隼人 > wrote: > > In your patch, the pg_dump.c was updated. IIUC the main reason was that > > pg_restore executes some queries as single transactions so that ALTER > > SUBSCRIPTION cannot be done, right? > > Yes, and please see below for other reasons. > > > Also, failover was synchronized only when we were in the upgrade mode, but > > your patch seems to remove the condition. Can you clarify the reason? > > We used ALTER SUBSCRIPTION in upgrade mode because it was not allowed to use > connect=false and failover=true together when CREATE SUBSCRIPTION. But since > we > don't have this restriction anymore(we don't alter slot when creating sub > anymore), we can directly specify failover in CREATE SUBSCRIPTION and do that > in non-upgrade mode as well. > > Attach the V3 patch which also addressed Shveta[1] and Bertrand[2]'s comments. Tested the patch, works well. thanks Shveta
Re: fix tablespace handling in pg_combinebackup
22.04.2024 00:49, Thomas Munro wrote:
On Mon, Apr 22, 2024 at 12:00 AM Alexander Lakhin wrote:
From what I can see, the following condition (namely, -l):
if ($path =~ /^pg_tblspc\/(\d+)$/ && -l "$backup_path/$path")
{
push @tsoids, $1;
return 0;
}
is false for junction points on Windows (cf [1]), but the target path is:
Ah, yes, right, -l doesn't like junction points. Well, we're already
using the Win32API::File package (it looks like a CPAN library, but I
guess the Windows perl distros like Strawberry are all including it
for free?). See PostgreSQL::Test::Utils::is_symlink(), attached.
That seems to work as expected, but someone who actually knows perl
can surely make it better. Then I hit the next problem:
readlink
C:\cirrus\build/testrun/pg_combinebackup/002_compare_backups\data/t_002_compare_backups_primary_data/backup/backup1/pg_tblspc/16415:
Inappropriate I/O control operation at
C:/cirrus/src/test/perl/PostgreSQL/Test/Cluster.pm line 927.
https://cirrus-ci.com/task/5162332353986560
I don't know where exactly that error message is coming from, but
assuming that Strawberry Perl contains this code:
https://github.com/Perl/perl5/blob/f936cd91ee430786a1bb6068a4a7c8362610dd5f/win32/win32.c#L2041
https://github.com/Perl/perl5/blob/f936cd91ee430786a1bb6068a4a7c8362610dd5f/win32/win32.c#L1976
... then it's *very* similar to what we're doing in our own
pgreadlink() code. I wondered if the buffer size might be too small
for our path, but it doesn't seem so:
https://github.com/Perl/perl5/blob/f936cd91ee430786a1bb6068a4a7c8362610dd5f/win32/win32.c#L1581C1-L1581C35
(I think MAX_PATH is 256 on Windows.)
If there is some unfixable problem with what they're doing in their
readlink(), then I guess it should be possible to read the junction
point directly in Perl using Win32API::File::DeviceIoControl()... but
I can't see what's wrong with it! Maybe it's not the same code?
I wonder whether the target path (\??\) of that junction point is fully correct.
I tried:
> mklink /j
"C:/src/postgresql/build/testrun/pg_combinebackup/002_compare_backups/data/t_002_compare_backups_primary_data/backup/backup1/pg_tblspc/test"
\\?\C:\t1
Junction created for
C:/src/postgresql/build/testrun/pg_combinebackup/002_compare_backups/data/t_002_compare_backups_primary_data/backup/backup1/pg_tblspc/test
<<===>> \\?\C:\t1
and
my $path =
'C:/src/postgresql/build/testrun/pg_combinebackup/002_compare_backups/data/t_002_compare_backups_primary_data/backup/backup1/pg_tblspc/test';
my $result = readlink($path);
works for me:
result: \\?\C:\t1
Whilst with:
my $path =
'C:/src/postgresql/build/testrun/pg_combinebackup/002_compare_backups/data/t_002_compare_backups_primary_data/backup/backup1/pg_tblspc/16415';
readlink() fails with "Invalid argument".
> dir
"C:/src/postgresql/build/testrun/pg_combinebackup/002_compare_backups/data/t_002_compare_backups_primary_data/backup/backup1/pg_tblspc"
04/22/2024 08:16 AM .
04/22/2024 08:16 AM ..
04/22/2024 06:52 AM 16415
[\??\C:\Users\ADMINI~1\AppData\Local\Temp\1zznr8FW5N\ts1backup]
04/22/2024 08:16 AM test [\\?\C:\t1]
> dir "\??\C:\Users\ADMINI~1\AppData\Local\Temp\1zznr8FW5N\ts1backup"
Directory of C:\??\C:\Users\ADMINI~1\AppData\Local\Temp\1zznr8FW5N\ts1backup
File Not Found
> dir "\\?\C:\t1"
Directory of \\?\C:\t1
04/22/2024 08:06 AM .
04/22/2024 08:06 AM ..
0 File(s) 0 bytes
Though
> dir
"C:/src/postgresql/build/testrun/pg_combinebackup/002_compare_backups/data/t_002_compare_backups_primary_data/backup/backup1/pg_tblspc/16415"
somehow really works:
Directory of
C:\src\postgresql\build\testrun\pg_combinebackup\002_compare_backups\data\t_002_compare_backups_primary_data\backup\backup1\pg_tblspc\16415
04/22/2024 06:52 AM .
04/22/2024 06:52 AM ..
04/22/2024 06:52 AM PG_17_202404021
Best regards,
Alexander
