[PoC] pg_upgrade: allow to upgrade publisher node
Dear hackers, (CC: Amit and Julien) This is a fork thread of Julien's thread, which allows to upgrade subscribers without losing changes [1]. I briefly implemented a prototype for allowing to upgrade publisher node. IIUC the key lack was that replication slots used for logical replication could not be copied to new node by pg_upgrade command, so this patch allows that. This feature can be used when '--include-replication-slot' is specified. Also, I added a small test for the typical case. It may be helpful to understand. Pg_upgrade internally executes pg_dump for dumping a database object from the old. This feature follows this, adds a new option '--slot-only' to pg_dump command. When specified, it extracts needed info from old node and generate an SQL file that executes pg_create_logical_replication_slot(). The notable deference from pre-existing is that restoring slots are done at the different time. Currently pg_upgrade works with following steps: ... 1. dump schema from old nodes 2. do pg_resetwal several times to new node 3. restore schema to new node 4. do pg_resetwal again to new node ... The probem is that if we create replication slots at step 3, the restart_lsn and confirmed_flush_lsn are set to current_wal_insert_lsn at that time, whereas pg_resetwal discards the WAL file. Such slots cannot extracting changes. To handle the issue the resotring is seprarated into two phases. At the first phase restoring is done at step 3, excepts replicatin slots. At the second phase replication slots are restored at step 5, after doing pg_resetwal. Before upgrading a publisher node, all the changes gerenated on publisher must be sent and applied on subscirber. This is because restart_lsn and confirmed_flush_lsn of copied replication slots is same as current_wal_insert_lsn. New node resets the information which WALs are really applied on subscriber and restart. Basically it is not problematic because before shutting donw the publisher, its walsender processes confirm all data is replicated. See WalSndDone() and related code. Currently physical slots are ignored because this is out-of-scope for me. I did not any analysis about it. [1]: https://www.postgresql.org/message-id/flat/20230217075433.u5mjly4d5cr4hcfe%40jrouhaud Best Regards, Hayato Kuroda FUJITSU LIMITED 0001-pg_upgrade-Add-include-replication-slot-option.patch Description: 0001-pg_upgrade-Add-include-replication-slot-option.patch
Re: Why enable_hashjoin Completely disables HashJoin
On 2023/4/3 19:44, Tomas Vondra wrote: On 4/3/23 12:23, Quan Zongliang wrote: Hi, I found that the enable_hashjoin disables HashJoin completely. It's in the function add_paths_to_joinrel: if (enable_hashjoin || jointype == JOIN_FULL) hash_inner_and_outer(root, joinrel, outerrel, innerrel, jointype, &extra); Instead, it should add a disable cost to the cost calculation of hashjoin. And now final_cost_hashjoin does the same thing: if (!enable_hashjoin) startup_cost += disable_cost; enable_mergejoin has the same problem. Test case: CREATE TABLE t_score_01( s_id int, s_score int, s_course char(8), c_id int); CREATE TABLE t_student_01( s_id int, s_name char(8)); insert into t_score_01 values( generate_series(1, 100), random()*100, 'course', generate_series(1, 100)); insert into t_student_01 values(generate_series(1, 100), 'name'); analyze t_score_01; analyze t_student_01; SET enable_hashjoin TO off; SET enable_nestloop TO off; SET enable_mergejoin TO off; explain select count(*) from t_student_01 a join t_score_01 b on a.s_id=b.s_id; After disabling all three, the HashJoin path should still be chosen. It's not clear to me why that behavior would be desirable? Why is this an issue you need so solve? Because someone noticed that when he set enable_hashjoin, enable_mergejoin and enable_nestloop to off. The statement seemed to get stuck (actually because it chose the NestedLoop path, which took a long long time to run). If enable_hashjoin and enable_nestloop disable generating these two paths. Then enable_nestloop should do the same thing, but it doesn't. AFAIK the reason why some paths are actually disabled (not built at all) while others are only penalized by adding disable_cost is that we need to end up with at least one way to execute the query. So we pick a path that we know is possible (e.g. seqscan) and hard-disable other paths. But the always-possible path is only soft-disabled by disable_cost. For joins, we do the same thing. The hash/merge joins may not be possible, because the data types may not have hash/sort operators, etc. Nestloop is always possible. So we soft-disable nestloop but hard-disable hash/merge joins. I doubt we want to change this behavior, unless there's a good reason to do that ... It doesn't have to change. Because selecting NestedLoop doesn't really get stuck either. It just takes too long to run. I will change the patch status to Withdrawn. regards
Re: Minimal logical decoding on standbys
On Tue, Apr 4, 2023 at 10:55 AM Masahiko Sawada wrote: > > On Tue, Apr 4, 2023 at 3:17 AM Drouvot, Bertrand > wrote: > > > > Hi, > > > > On 4/3/23 8:10 AM, Drouvot, Bertrand wrote: > > > Hi, > > > > > > On 4/3/23 7:35 AM, Amit Kapila wrote: > > >> On Mon, Apr 3, 2023 at 4:26 AM Jeff Davis wrote: > > >> > > >> Agreed, even Bertrand and myself discussed the same approach few > > >> emails above. BTW, if we have this selective logic to wake > > >> physical/logical walsenders and for standby's, we only wake logical > > >> walsenders at the time of ApplyWalRecord() then do we need the new > > >> conditional variable enhancement being discussed, and if so, why? > > >> > > > > > > Thank you both for this new idea and discussion. In that case I don't > > > think > > > we need the new CV API and the use of a CV anymore. As just said > > > up-thread I'll submit > > > a new proposal with this new approach. > > > > > > > Please find enclosed V57 implementing the new approach in 0004. > > Regarding 0004 patch: > > @@ -2626,6 +2626,12 @@ InitWalSenderSlot(void) > walsnd->sync_standby_priority = 0; > walsnd->latch = &MyProc->procLatch; > walsnd->replyTime = 0; > + > + if (MyDatabaseId == InvalidOid) > + walsnd->kind = REPLICATION_KIND_PHYSICAL; > + else > + walsnd->kind = REPLICATION_KIND_LOGICAL; > + > > I think we might want to set the replication kind when processing the > START_REPLICATION command. The walsender using a logical replication > slot is not necessarily streaming (e.g. when COPYing table data). > Discussing with Bertrand off-list, it's wrong as the logical replication slot creation also needs to read WAL records so a walsender who is creating a logical replication slot needs to be woken up. We can set it the replication kind when processing START_REPLICATION and CREATE_REPLICATION_SLOT, but it seems better to set it in one place. So I agree to set it in InitWalSenderSlot(). Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: [PATCH] Clarify the behavior of the system when approaching XID wraparound
On Mon, Apr 3, 2023 at 7:33 PM Aleksander Alekseev
wrote:
> > Yes, the exact same text as it appeared in the [2] thread above, which
prompted Robert's comment I quoted. I take the point to mean: All of these
things need to be taken care of *first*, before vacuuming, so the hint
should order things so that it is clear.
> >
> > > Please let me know if you think
> > > we should also add a suggestion to kill long-running sessions, etc.
> >
> > +1 for also adding that.
>
> OK, done. I included this change as a separate patch. It can be
> squashed with another one if necessary.
Okay, great. For v4-0003:
Each hint mentions vacuum twice, because it's adding the vacuum message at
the end, but not removing it from the beginning. The vacuum string should
be on its own line, since we will have to modify that for back branches
(skip indexes and truncation).
I'm also having second thoughts about "Ensure that autovacuum is
progressing" in the hint. That might be better in the docs, if we decide to
go ahead with adding a specific checklist there.
In vacuum.c:
errhint("Close open transactions soon to avoid wraparound problems.\n"
- "You might also need to commit or roll back old prepared transactions, or
drop stale replication slots.")));
+ "You might also need to commit or roll back old prepared transactions,
drop stale replication slots, or kill long-running sessions. Ensure that
autovacuum is progressing, or run a manual database-wide VACUUM.")));
This appears in vacuum_get_cutoffs(), which is called by vacuum and
cluster, and the open transactions were already mentioned, so this is not
the place for this change.
> This particular wording was chosen for consistency with multixact.c:
>
> ```
> errmsg("database is not accepting commands that generate new
> MultiXactIds to avoid wraparound data loss in database \"%s\"",
> ```
Okay, I didn't look into that -- seems like a good precedent.
v4-0002:
- errhint("Stop the postmaster and vacuum that database in single-user
mode.\n"
+ errhint("VACUUM that database.\n"
Further in the spirit of consistency, the mulixact path already has
"Execute a database-wide VACUUM in that database.\n", and that seems like
better wording.
--
John Naylor
EDB: http://www.enterprisedb.com
Re: Should vacuum process config file reload more often
On Tue, Apr 4, 2023 at 1:41 AM Melanie Plageman wrote: > > On Sun, Apr 2, 2023 at 10:28 PM Masahiko Sawada wrote: > > Thank you for updating the patches. Here are comments for 0001, 0002, > > and 0003 patches: > > Thanks for the review! > > v13 attached with requested updates. > > > 0001: > > > > @@ -391,7 +389,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, > > Assert(params->index_cleanup != VACOPTVALUE_UNSPECIFIED); > > Assert(params->truncate != VACOPTVALUE_UNSPECIFIED && > > params->truncate != VACOPTVALUE_AUTO); > > -vacrel->failsafe_active = false; > > +VacuumFailsafeActive = false; > > > > If we go with the idea of using VacuumCostActive + > > VacuumFailsafeActive, we need to make sure that both are cleared at > > the end of the vacuum per table. Since the patch clears it only here, > > it remains true even after vacuum() if we trigger the failsafe mode > > for the last table in the table list. > > > > In addition to that, to ensure that also in an error case, I think we > > need to clear it also in PG_FINALLY() block in vacuum(). > > So, in 0001, I tried to keep it exactly the same as > LVRelState->failsafe_active except for it being a global. We don't > actually use VacuumFailsafeActive in this commit except in vacuumlazy.c, > which does its own management of the value (it resets it to false at the > top of heap_vacuum_rel()). > > In the later commit which references VacuumFailsafeActive outside of > vacuumlazy.c, I had reset it in PG_FINALLY(). I hadn't reset it in the > relation list loop in vacuum(). Autovacuum calls vacuum() for each > relation. However, you are right that for VACUUM with a list of > relations for a table access method other than heap, once set to true, > if the table AM forgets to reset the value to false at the end of > vacuuming the relation, it would stay true. > > I've set it to false now at the bottom of the loop through relations in > vacuum(). Agreed. Probably we can merge 0001 into 0003 but I leave it to committers. The 0001 patch mostly looks good to me except for one point: @@ -391,7 +389,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, Assert(params->index_cleanup != VACOPTVALUE_UNSPECIFIED); Assert(params->truncate != VACOPTVALUE_UNSPECIFIED && params->truncate != VACOPTVALUE_AUTO); -vacrel->failsafe_active = false; +VacuumFailsafeActive = false; vacrel->consider_bypass_optimization = true; vacrel->do_index_vacuuming = true; Looking at the 0003 patch, we set VacuumFailsafeActive to false per table: +/* + * Ensure VacuumFailsafeActive has been reset before vacuuming the + * next relation relation. + */ +VacuumFailsafeActive = false; Given that we ensure it's reset before vacuuming the next table, do we need to reset it in heap_vacuum_rel? (there is a typo; s/relation relation/relation/) > > > --- > > @@ -306,6 +306,7 @@ extern PGDLLIMPORT pg_atomic_uint32 > > *VacuumSharedCostBalance; > > extern PGDLLIMPORT pg_atomic_uint32 *VacuumActiveNWorkers; > > extern PGDLLIMPORT int VacuumCostBalanceLocal; > > > > +extern bool VacuumFailsafeActive; > > > > Do we need PGDLLIMPORT for VacuumFailSafeActive? > > I didn't add one because I thought extensions and other code probably > shouldn't access this variable. I thought PGDLLIMPORT was only needed > for extensions built on windows to access variables. Agreed. > > > 0002: > > > > @@ -2388,6 +2398,7 @@ vac_max_items_to_alloc_size(int max_items) > > return offsetof(VacDeadItems, items) + > > sizeof(ItemPointerData) * max_items; > > } > > > > + > > /* > > * vac_tid_reaped() -- is a particular tid deletable? > > * > > > > Unnecessary new line. There are some other unnecessary new lines in this > > patch. > > Thanks! I think I got them all. > > > --- > > @@ -307,6 +309,8 @@ extern PGDLLIMPORT pg_atomic_uint32 > > *VacuumActiveNWorkers; > > extern PGDLLIMPORT int VacuumCostBalanceLocal; > > > > extern bool VacuumFailsafeActive; > > +extern int VacuumCostLimit; > > +extern double VacuumCostDelay; > > > > and > > > > @@ -266,8 +266,6 @@ extern PGDLLIMPORT int max_parallel_maintenance_workers; > > extern PGDLLIMPORT int VacuumCostPageHit; > > extern PGDLLIMPORT int VacuumCostPageMiss; > > extern PGDLLIMPORT int VacuumCostPageDirty; > > -extern PGDLLIMPORT int VacuumCostLimit; > > -extern PGDLLIMPORT double VacuumCostDelay; > > > > Do we need PGDLLIMPORT too? > > I was on the fence about this. I annotated the new guc variables > vacuum_cost_delay and vacuum_cost_limit with PGDLLIMPORT, but I did not > annotate the variables used in vacuum code (VacuumCostLimit/Delay). I > think whether or not this is the right choice depends on two things: > whether or not my understanding of PGDLLIMPORT is correct and, if it is, > whether or not we
Re: Minimal logical decoding on standbys
Hi,
On 4/4/23 7:57 AM, Amit Kapila wrote:
On Mon, Apr 3, 2023 at 8:51 PM Drouvot, Bertrand
wrote:
I made it as simple as:
/*
* If the slot is already invalid or is a non conflicting slot, we
don't
* need to do anything.
*/
islogical = xid ? true : false;
if (SlotIsInvalid(s, islogical) || SlotIsNotConflicting(s, islogical,
xid, &oldestLSN))
in V56 attached.
Here the variable 'islogical' doesn't seem to convey its actual
meaning because one can imagine that it indicates whether the slot is
logical which I don't think is the actual intent.
Good point. Just renamed it to 'check_on_xid' (as still needed outside of
the "CanInvalidateSlot" context) in V58 attached.
One idea to simplify
this is to introduce a single function CanInvalidateSlot() or
something like that and move the logic from both the functions
SlotIsInvalid() and SlotIsNotConflicting() into the new function.
Oh right, even better, thanks!
Done in V58 and now this is as simple as:
+ if (DoNotInvalidateSlot(s, xid, &oldestLSN))
{
/* then, we are not forcing for invalidation */
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.comFrom 7c61edd93b4df1efb9723ddae41c009ccbac9f59 Mon Sep 17 00:00:00 2001
From: bdrouvotAWS
Date: Tue, 7 Feb 2023 14:08:11 +
Subject: [PATCH v58 6/6] Doc changes describing details about logical
decoding.
Author: Andres Freund (in an older version), Amit Khandekar, Bertrand Drouvot
Reviewed-By: Bertrand Drouvot, Andres Freund, Robert Haas, Fabrizio de Royes
Mello
---
doc/src/sgml/logicaldecoding.sgml | 22 ++
1 file changed, 22 insertions(+)
100.0% doc/src/sgml/
diff --git a/doc/src/sgml/logicaldecoding.sgml
b/doc/src/sgml/logicaldecoding.sgml
index 4e912b4bd4..3da254ed1f 100644
--- a/doc/src/sgml/logicaldecoding.sgml
+++ b/doc/src/sgml/logicaldecoding.sgml
@@ -316,6 +316,28 @@ postgres=# select * from
pg_logical_slot_get_changes('regression_slot', NULL, NU
may consume changes from a slot at any given time.
+
+ A logical replication slot can also be created on a hot standby. To
prevent
+ VACUUM from removing required rows from the system
+ catalogs, hot_standby_feedback should be set on the
+ standby. In spite of that, if any required rows get removed, the slot gets
+ invalidated. It's highly recommended to use a physical slot between the
primary
+ and the standby. Otherwise, hot_standby_feedback will work, but only
while the
+ connection is alive (for example a node restart would break it). Existing
+ logical slots on standby also get invalidated if wal_level on primary is
reduced to
+ less than 'logical'.
+
+
+
+ For a logical slot to be created, it builds a historic snapshot, for which
+ information of all the currently running transactions is essential. On
+ primary, this information is available, but on standby, this information
+ has to be obtained from primary. So, slot creation may wait for some
+ activity to happen on the primary. If the primary is idle, creating a
+ logical slot on standby may take a noticeable time. One option to speed it
+ is to call the pg_log_standby_snapshot on the
primary.
+
+
Replication slots persist across crashes and know nothing about the state
--
2.34.1
From 14f9f51aaacd8889ef1f9853534c9303fc89f59f Mon Sep 17 00:00:00 2001
From: bdrouvotAWS
Date: Tue, 7 Feb 2023 09:04:12 +
Subject: [PATCH v58 5/6] New TAP test for logical decoding on standby.
In addition to the new TAP test, this commit introduces a new
pg_log_standby_snapshot()
function.
The idea is to be able to take a snapshot of running transactions and write this
to WAL without requesting for a (costly) checkpoint.
Author: Craig Ringer (in an older version), Amit Khandekar, Bertrand Drouvot
Reviewed-By: Bertrand Drouvot, Andres Freund, Robert Haas, Fabrizio de Royes
Mello
---
doc/src/sgml/func.sgml| 15 +
src/backend/access/transam/xlogfuncs.c| 32 +
src/backend/catalog/system_functions.sql | 2 +
src/include/catalog/pg_proc.dat | 3 +
src/test/perl/PostgreSQL/Test/Cluster.pm | 37 +
src/test/recovery/meson.build | 1 +
.../t/035_standby_logical_decoding.pl | 705 ++
7 files changed, 795 insertions(+)
3.1% src/backend/
4.0% src/test/perl/PostgreSQL/Test/
89.7% src/test/recovery/t/
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 918a492234..939fb8019f 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -27034,6 +27034,21 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number *
ps.setting::int + :offset
prepared with .
+
+
+
+ pg_log_standby_snapshot
+
+
Re: Minimal logical decoding on standbys
Hi, On 4/4/23 9:48 AM, Masahiko Sawada wrote: On Tue, Apr 4, 2023 at 10:55 AM Masahiko Sawada wrote: Regarding 0004 patch: @@ -2626,6 +2626,12 @@ InitWalSenderSlot(void) walsnd->sync_standby_priority = 0; walsnd->latch = &MyProc->procLatch; walsnd->replyTime = 0; + + if (MyDatabaseId == InvalidOid) + walsnd->kind = REPLICATION_KIND_PHYSICAL; + else + walsnd->kind = REPLICATION_KIND_LOGICAL; + I think we might want to set the replication kind when processing the START_REPLICATION command. The walsender using a logical replication slot is not necessarily streaming (e.g. when COPYing table data). Discussing with Bertrand off-list, it's wrong as the logical replication slot creation also needs to read WAL records so a walsender who is creating a logical replication slot needs to be woken up. We can set it the replication kind when processing START_REPLICATION and CREATE_REPLICATION_SLOT, but it seems better to set it in one place. So I agree to set it in InitWalSenderSlot(). Thanks for the review and feedback! Added a comment in 0004 in V58 just posted up-thread to explain the reason why the walsnd->kind assignment is done InitWalSenderSlot(). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: [PATCH] Clarify the behavior of the system when approaching XID wraparound
Hi! I've looked into the patches v4. For 0001: I think long "not accepting commands that generate" is equivalent to more concise "can't generate". For 0003: I think double mentioning of Vacuum at each errhist i.e.: "Execute a database-wide VACUUM in that database" and "...or run a manual database-wide VACUUM." are redundant. The advice "Ensure that autovacuum is progressing,..." is also not needed after advice to "Execute a database-wide VACUUM in that database". For all: In a errhint's list what _might_ be done I think AND is a little bit better that OR as the word _might_ means that each of the proposals in the list is a probable, not a sure one. The proposed changes are in patchset v5. Kind regards, Pavel Borisov, Supabase. v5-0002-This-recommendation-is-outdated-for-some-time-now.patch Description: Binary data v5-0001-Correct-the-docs-and-messages-about-preventing-XI.patch Description: Binary data v5-0003-Modify-the-hints-about-preventing-XID-wraparound.patch Description: Binary data
Re: Split index and table statistics into different types of stats
Hi, On 4/3/23 11:47 PM, Gregory Stark (as CFM) wrote: On Thu, 16 Mar 2023 at 05:25, Drouvot, Bertrand wrote: My plan was to get [1] done before resuming working on the "Split index and table statistics into different types of stats" one. [1]: https://commitfest.postgresql.org/42/4106/ Generate pg_stat_get_xact*() functions with Macros ([1]) was committed March 27. There's only a few days left in this CF. Would you like to leave this here? Should it be marked Needs Review or Ready for Commit? Or should we move it to the next CF now? I moved it to the next commitfest and marked the target version as v17. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
[PATCH] Add statement_timeout in pg_stat_activity
Hello hackers.
This patch adds the backend's statement_timeout value to pg_stat_activity.
This would provide some insights on clients that are disabling a default
statement timeout or overriding it through a pgbouncer, messing with other
sessions.
pg_stat_activity seemed like the best place to have this information.
Regards,
Anthonin
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index d5a45f996d..2109595c40 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -861,6 +861,15 @@ postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser
+
+
+ statement_timeout integer
+
+
+ The current backend's statement_timeout.
+
+
+
wait_event_type text
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 574cbc2e44..7c24ef92bf 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -860,6 +860,7 @@ CREATE VIEW pg_stat_activity AS
S.xact_start,
S.query_start,
S.state_change,
+S.statement_timeout,
S.wait_event_type,
S.wait_event,
S.state,
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index f0f2e07655..0a2ba26070 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -1054,6 +1054,16 @@ assign_application_name(const char *newval, void *extra)
pgstat_report_appname(newval);
}
+/*
+ * GUC assign_hook for statement_timeout
+ */
+void
+assign_statement_timeout(int newval, void *extra)
+{
+ /* Update the pg_stat_activity view */
+ pgstat_report_statement_timeout(newval);
+}
+
/*
* GUC check_hook for cluster_name
*/
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index 608d01ea0d..5d94cb0ba6 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -336,6 +336,7 @@ pgstat_bestart(void)
lbeentry.st_activity_start_timestamp = 0;
lbeentry.st_state_start_timestamp = 0;
lbeentry.st_xact_start_timestamp = 0;
+ lbeentry.st_statement_timeout = 0;
lbeentry.st_databaseid = MyDatabaseId;
/* We have userid for client-backends, wal-sender and bgworker processes */
@@ -446,9 +447,11 @@ pgstat_bestart(void)
PGSTAT_END_WRITE_ACTIVITY(vbeentry);
- /* Update app name to current GUC setting */
- if (application_name)
+ /* Update app name and statement timeout to current GUC setting */
+ if (application_name) {
pgstat_report_appname(application_name);
+ pgstat_report_statement_timeout(StatementTimeout);
+ }
}
/*
@@ -692,6 +695,33 @@ pgstat_report_appname(const char *appname)
PGSTAT_END_WRITE_ACTIVITY(beentry);
}
+/* --
+ * pgstat_report_statement_timeout() -
+ *
+ * Called to update statement_timeout.
+ * --
+ */
+void
+pgstat_report_statement_timeout(int statement_timeout)
+{
+ volatile PgBackendStatus *beentry = MyBEEntry;
+
+ if (!beentry)
+ return;
+
+ /*
+ * Update my status entry, following the protocol of bumping
+ * st_changecount before and after. We use a volatile pointer here to
+ * ensure the compiler doesn't try to get cute.
+ */
+ PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
+
+ beentry->st_statement_timeout = statement_timeout;
+
+ PGSTAT_END_WRITE_ACTIVITY(beentry);
+}
+
+
/*
* Report current transaction start timestamp as the specified value.
* Zero means there is no active transaction.
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index eec9f3cf9b..de72c2c1d0 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -303,7 +303,7 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS)
Datum
pg_stat_get_activity(PG_FUNCTION_ARGS)
{
-#define PG_STAT_GET_ACTIVITY_COLS 30
+#define PG_STAT_GET_ACTIVITY_COLS 31
int num_backends = pgstat_fetch_stat_numbackends();
int curr_backend;
int pid = PG_ARGISNULL(0) ? -1 : PG_GETARG_INT32(0);
@@ -612,6 +612,12 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
nulls[29] = true;
else
values[29] = UInt64GetDatum(beentry->st_query_id);
+ /* Only report statement_timeout for client backend */
+ if (beentry->st_backendType == B_BACKEND) {
+values[30] = Int32GetDatum(beentry->st_statement_timeout);
+ } else {
+nulls[30] = true;
+ }
}
else
{
@@ -640,6 +646,7 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
nulls[27] = true;
nulls[28] = true;
nulls[29] = true;
+ nulls[30] = true;
}
tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, values, nulls);
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 8062589efd..5dd33f583f 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2478,7 +2478,7 @@ struct config_int ConfigureNamesInt[] =
Re: Patch proposal: New hooks in the connection path
Hi, On 4/4/23 12:08 AM, Gregory Stark (as CFM) wrote: This looks like it was a good discussion -- last summer. But it doesn't seem to be a patch under active development now. It sounds like there were some design constraints that still need some new ideas to solve and a new patch will be needed to address them. Should this be marked Returned With Feedback? I just marked it as Returned With Feedback. I may re-open it later on to resume the discussion or share new ideas though. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: postgres_fdw: commit remote (sub)transactions in parallel during pre-commit
On Wed, Jan 18, 2023 at 8:06 PM Etsuro Fujita wrote: > I rebased the patch. Attached is an updated patch. The parallel-abort patch received a review from David, and I addressed his comments. Also, he tested with the patch, and showed that it reduces time taken to abort remote transactions. So, if there are no objections, I will commit the patch. Best regards, Etsuro Fujita
Re: Split index and table statistics into different types of stats
On Tue, Apr 04, 2023 at 12:04:35PM +0200, Drouvot, Bertrand wrote: > I moved it to the next commitfest and marked the target version as > v17. Thanks for moving it. I think that we should be able to do a bit more work for the switch to macros in pgstatfuncs.c, but this is going to require more review than the feature freeze date allow, I am afraid. -- Michael signature.asc Description: PGP signature
Re: SQL/JSON revisited
Hi Alvaro, On Tue, Apr 4, 2023 at 2:16 AM Alvaro Herrera wrote: > On 2023-Mar-29, Alvaro Herrera wrote: > > In the meantime, here's the next two patches of the series: IS JSON and > > the "query" functions. I think this is as much as I can get done for > > this release, so the last two pieces of functionality would have to wait > > for 17. I still need to clean these up some more. These are not > > thoroughly tested either; 0001 compiles and passes regression tests, but > > I didn't verify 0003 other than there being no Git conflicts and bison > > doesn't complain. > > > > Also, notable here is that I realized that I need to backtrack on my > > change of the WITHOUT_LA: the original patch had it for TIME (in WITHOUT > > TIME ZONE), and I changed to be for UNIQUE. But now that I've done > > "JSON query functions" I realize that it needed to be the other way for > > the WITHOUT ARRAY WRAPPER clause too. So 0002 reverts that choice. > > So I pushed 0001 on Friday, and here are 0002 (which I intend to push > shortly, since it shouldn't be controversial) and the "JSON query > functions" patch as 0003. After looking at it some more, I think there > are some things that need to be addressed by one of the authors: > > - the gram.y solution to the "ON ERROR/ON EMPTY" clauses is quite ugly. > I think we could make that stuff use something similar to > ConstraintAttributeSpec with an accompanying post-processing function. > That would reduce the number of ad-hoc hacks, which seem excessive. Do you mean the solution involving the JsonBehavior node? > - the changes in formatting.h have no explanation whatsoever. At the > very least, the new function should have a comment in the .c file. > (And why is it at end of file? I bet there's a better location) > > - some nasty hacks are being used in the ECPG grammar with no tests at > all. It's easy to add a few lines to the .pgc file I added in prior > commits. > > - Some functions in jsonfuncs.c have changed from throwing hard errors > into soft ones. I think this deserves more commentary. > > - func.sgml: The new functions are documented in a separate table for no > reason that I can see. Needs to be merged into one of the existing > tables. I didn't actually review the docs. I made the jsonfuncs.c changes to use soft error handling when needed, so I took a stab at that; attached a delta patch, which also fixes the problematic comments mentioned by Alexander in his comments 1 and 3. I'll need to spend some time to address other points. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com v15-0002-delta.patch Description: Binary data
Re: Minimal logical decoding on standbys
Hi, On 2023-Apr-03, Andres Freund wrote: > Hm? That's what the _'s do. We build strings in parts in other places too. No, what _() does is mark each piece for translation separately. But a translation cannot be done on string pieces, and later have all the pieces appended together to form a full sentence. Let me show the "!terminating" case as example and grab some translations for it from src/backend/po/de.po: "invalidating" -> "... wird ungültig gemacht" (?) (if logical) " obsolete replication" -> " obsolete Replikation" " slot \"%s\" because it conflicts with recovery" -> " Slot \"%s\", weil sie in Konflikt mit Wiederherstellung steht" If you just concatenate all the translated phrases together, the resulting string will make no sense; keep in mind the "obsolete replication" part may or not may not be there. And there's no way to make that work: even if you found an ordering of the English parts that allows you to translate each piece separately and have it make sense for German, the same won't work for Spanish or Japanese. You have to give the translator a complete phrase and let them turn into a complete translated phrases. Building from parts doesn't work. We're very good at avoiding string building; we have a couple of cases, but they are *very* minor. string 1 "invalidating slot \"%s\" because it conflicts with recovery" string 2 "invalidating obsolete replication slot \"%s\" because it conflicts with recovery" (I'm not clear on why did Bertrand omitted the word "replication" in the case where the slot is not logical) I think the errdetail() are okay, it's the errmsg() bits that are bogus. And yes, well caught on having to use errmsg_internal and errdetail_internal() to avoid double translation. Cheers -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Re: Support logical replication of DDLs
On Tue, Apr 4, 2023 at 8:43 AM houzj.f...@fujitsu.com wrote: > Attach the new version patch set which did the following changes: > Hello, I tried below: pubnew=# ALTER PUBLICATION mypub2 SET (ddl = 'table'); ALTER PUBLICATION pubnew=# \dRp+ Publication mypub2 Owner | All tables | All DDLs | Table DDLs | ++--++- shveta | t | f | f (1 row) I still see 'Table DDLs' as false and ddl replication did not work for this case. thanks Shveta
Re: Minimal logical decoding on standbys
On Tue, Apr 4, 2023 at 3:14 PM Drouvot, Bertrand
wrote:
>
+static inline bool
+LogicalReplicationSlotXidsConflict(ReplicationSlot *s, TransactionId xid)
+{
+ TransactionId slot_xmin;
+ TransactionId slot_catalog_xmin;
+
+ slot_xmin = s->data.xmin;
+ slot_catalog_xmin = s->data.catalog_xmin;
+
+ return (((TransactionIdIsValid(slot_xmin) &&
TransactionIdPrecedesOrEquals(slot_xmin, xid)) ||
For logical slots, slot->data.xmin will always be an
InvalidTransactionId. It will only be set/updated for physical slots.
So, it is not clear to me why in this and other related functions, you
are referring to and or invalidating it.
--
With Regards,
Amit Kapila.
Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
On Wed, Mar 29, 2023 at 07:44:20AM +0200, Drouvot, Bertrand wrote: > Please find a draft attached. This addition looks OK for me. Thanks for the patch! -- Michael signature.asc Description: PGP signature
Re: [BUG] pg_stat_statements and extended query protocol
On Tue, Apr 04, 2023 at 03:29:07AM +, Imseih (AWS), Sami wrote: >> We clearly do need to fix the >> reported rowcount for cases where ExecutorRun is invoked more than >> once per ExecutorEnd call; but I think that's sufficient. > > Sure, the original proposed fix, but with tracking the es_total_processed > only in Estate should be enough for now. I was looking back at this thread, and the suggestion to use one field in EState sounds fine to me. Sami, would you like to send a new version of the patch (simplified version based on v1)? -- Michael signature.asc Description: PGP signature
Re: fairywren exiting in ecpg
On 2023-04-03 Mo 21:15, Andres Freund wrote: Hi, Looks like fairywren is possibly seeing something I saw before and spent many days looking into: https://postgr.es/m/20220909235836.lz3igxtkcjb5w7zb%40awork3.anarazel.de which led me to add the following to .cirrus.yml: # Cirrus defaults to SetErrorMode(SEM_NOGPFAULTERRORBOX | ...). That # prevents crash reporting from working unless binaries do SetErrorMode() # themselves. Furthermore, it appears that either python or, more likely, # the C runtime has a bug where SEM_NOGPFAULTERRORBOX can very # occasionally *trigger* a crash on process exit - which is hard to debug, # given that it explicitly prevents crash dumps from working... # 0x8001 is SEM_FAILCRITICALERRORS | SEM_NOOPENFILEERRORBOX CIRRUS_WINDOWS_ERROR_MODE: 0x8001 The mingw folks also spent a lot of time looking into this ([1]), without a lot of success. It sure looks like it might be a windows C runtime issue - none of the stacktrace handling python has gets invoked. I could not find any relevant behavoural differences in python's code that depend on SEM_NOGPFAULTERRORBOX being set. It'd be interesting to see if fairywren's occasional failures go away if you set MSYS=winjitdebug (which prevents msys from adding SEM_NOGPFAULTERRORBOX to ErrorMode). trying now. Since this happened every build or so it shouldn't take long for us to see. (I didn't see anything in the MSYS2 docs that specified the possible values for MSYS :-( ) cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: Minimal logical decoding on standbys
Hi,
On 4/4/23 1:43 PM, Amit Kapila wrote:
On Tue, Apr 4, 2023 at 3:14 PM Drouvot, Bertrand
wrote:
+static inline bool
+LogicalReplicationSlotXidsConflict(ReplicationSlot *s, TransactionId xid)
+{
+ TransactionId slot_xmin;
+ TransactionId slot_catalog_xmin;
+
+ slot_xmin = s->data.xmin;
+ slot_catalog_xmin = s->data.catalog_xmin;
+
+ return (((TransactionIdIsValid(slot_xmin) &&
TransactionIdPrecedesOrEquals(slot_xmin, xid)) ||
For logical slots, slot->data.xmin will always be an
InvalidTransactionId. It will only be set/updated for physical slots.
So, it is not clear to me why in this and other related functions, you
are referring to and or invalidating it.
I think you're right that invalidating/checking only on the catalog xmin is
enough for logical slot (I'm not sure how I ended up taking the xmin into
account but
that seems useless indeed).
I'll submit a new version to deal with the catalog xmin only, thanks!
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Re: SQL/JSON revisited
On 2023-Apr-04, Amit Langote wrote:
> On Tue, Apr 4, 2023 at 2:16 AM Alvaro Herrera wrote:
> > - the gram.y solution to the "ON ERROR/ON EMPTY" clauses is quite ugly.
> > I think we could make that stuff use something similar to
> > ConstraintAttributeSpec with an accompanying post-processing function.
> > That would reduce the number of ad-hoc hacks, which seem excessive.
>
> Do you mean the solution involving the JsonBehavior node?
Right. It has spilled as the separate on_behavior struct in the core
parser %union in addition to the raw jsbehavior, which is something
we've gone 30 years without having, and I don't see why we should start
now.
This stuff is terrible:
json_exists_error_clause_opt:
json_exists_error_behavior ON ERROR_P { $$ = $1; }
| /* EMPTY */ { $$ = NULL; }
;
json_exists_error_behavior:
ERROR_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR, NULL); }
| TRUE_P{ $$ = makeJsonBehavior(JSON_BEHAVIOR_TRUE, NULL); }
| FALSE_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_FALSE, NULL);
}
| UNKNOWN { $$ = makeJsonBehavior(JSON_BEHAVIOR_UNKNOWN,
NULL); }
;
json_value_behavior:
NULL_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_NULL, NULL); }
| ERROR_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR, NULL);
}
| DEFAULT a_expr{ $$ = makeJsonBehavior(JSON_BEHAVIOR_DEFAULT,
$2); }
;
json_value_on_behavior_clause_opt:
json_value_behavior ON EMPTY_P
{ $$.on_empty = $1; $$.on_error = NULL; }
| json_value_behavior ON EMPTY_P json_value_behavior ON ERROR_P
{ $$.on_empty = $1; $$.on_error = $4; }
| json_value_behavior ON ERROR_P
{ $$.on_empty = NULL; $$.on_error = $1; }
| /* EMPTY */
{ $$.on_empty = NULL; $$.on_error = NULL; }
;
json_query_behavior:
ERROR_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR, NULL); }
| NULL_P{ $$ = makeJsonBehavior(JSON_BEHAVIOR_NULL, NULL); }
| EMPTY_P ARRAY { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY,
NULL); }
/* non-standard, for Oracle compatibility only */
| EMPTY_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY,
NULL); }
| EMPTY_P OBJECT_P { $$ =
makeJsonBehavior(JSON_BEHAVIOR_EMPTY_OBJECT, NULL); }
| DEFAULT a_expr{ $$ = makeJsonBehavior(JSON_BEHAVIOR_DEFAULT,
$2); }
;
json_query_on_behavior_clause_opt:
json_query_behavior ON EMPTY_P
{ $$.on_empty = $1; $$.on_error = NULL; }
| json_query_behavior ON EMPTY_P json_query_behavior ON ERROR_P
{ $$.on_empty = $1; $$.on_error = $4; }
| json_query_behavior ON ERROR_P
{ $$.on_empty = NULL; $$.on_error = $1; }
| /* EMPTY */
{ $$.on_empty = NULL; $$.on_error = NULL; }
;
Surely this can be made cleaner.
By the way -- that comment about clauses being non-standard, can you
spot exactly *which* clauses that comment applies to?
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"El número de instalaciones de UNIX se ha elevado a 10,
y se espera que este número aumente" (UPM, 1972)
Check whether binaries can be released for s390x
Hi, We are looking forward to get help from community on publishing s390x binaries. As per downloads page, apt repo supports Ubuntu on amd,arm,i386 and ppc64le. We had reached out earlier and are ready to provide infra if needed. Wanted to check again if community is willing to help. Thank you, Namrata
Re: [PATCH] Clarify the behavior of the system when approaching XID wraparound
Hi,
> The proposed changes are in patchset v5.
Pavel, John, thanks for your feedback.
> I've looked into the patches v4.
> For 0001:
> I think long "not accepting commands that generate" is equivalent to
> more concise "can't generate".
Frankly I don't think this is a good change for this particular CF
entry and it violates the consistency with multixact.c. Additionally
the new message is not accurate. The DBMS _can_ generate new XIDs,
quite a few of them actually. It merely refuses to do so.
> For all:
> In a errhint's list what _might_ be done I think AND is a little bit
> better that OR as the word _might_ means that each of the proposals in
> the list is a probable, not a sure one.
Well, that's debatable... IMO "or" makes a bit more sense since the
user knows better whether he/she needs to kill a long-running
transaction, or run VACUUM, or maybe do both. "And" would imply that
the user needs to do all of it, which is not necessarily true. Since
originally it was "or" I suggest we keep it as is for now.
All in all I would prefer keeping the focus on the particular problem
the patch tries to address.
> For 0003:
> I think double mentioning of Vacuum at each errhist i.e.: "Execute a
> database-wide VACUUM in that database" and "...or run a manual
> database-wide VACUUM." are redundant. The advice "Ensure that
> autovacuum is progressing,..." is also not needed after advice to
> "Execute a database-wide VACUUM in that database".
> [...]
> Okay, great. For v4-0003:
>
> Each hint mentions vacuum twice, because it's adding the vacuum message at
> the end, but not removing it from the beginning. The vacuum string should be
> on its own line, since we will have to modify that for back branches (skip
> indexes and truncation).
My bad. Fixed.
> I'm also having second thoughts about "Ensure that autovacuum is progressing"
> in the hint. That might be better in the docs, if we decide to go ahead with
> adding a specific checklist there.
OK, done.
> In vacuum.c:
>
> errhint("Close open transactions soon to avoid wraparound problems.\n"
> - "You might also need to commit or roll back old prepared transactions, or
> drop stale replication slots.")));
> + "You might also need to commit or roll back old prepared transactions, drop
> stale replication slots, or kill long-running sessions. Ensure that
> autovacuum is progressing, or run a manual database-wide VACUUM.")));
>
> This appears in vacuum_get_cutoffs(), which is called by vacuum and cluster,
> and the open transactions were already mentioned, so this is not the place
> for this change.
Fixed.
> v4-0002:
>
> - errhint("Stop the postmaster and vacuum that database in single-user
> mode.\n"
> + errhint("VACUUM that database.\n"
>
> Further in the spirit of consistency, the mulixact path already has "Execute
> a database-wide VACUUM in that database.\n", and that seems like better
> wording.
Agree. Fixed.
--
Best regards,
Aleksander Alekseev
v6-0001-Correct-the-docs-and-messages-about-preventing-XI.patch
Description: Binary data
v6-0002-Don-t-recommend-running-VACUUM-in-a-single-user-m.patch
Description: Binary data
v6-0003-Modify-the-hints-about-preventing-XID-wraparound.patch
Description: Binary data
Re: ICU locale validation / canonicalization
On 31.03.23 12:11, Jeff Davis wrote: On Thu, 2023-03-30 at 08:59 +0200, Peter Eisentraut wrote: I don't think we should show the notice when the canonicalization doesn't change anything. This is not useful: +NOTICE: using language tag "und-u-kf-upper" for locale "und-u-kf- upper" Done. Also, the message should be phrased more from the perspective of the user instead of using ICU jargon, like NOTICE: using canonicalized form "%s" for locale specification "%s" (Still too many big words?) Changed to: NOTICE: using standard form "%s" for locale "%s" Needs documentation updates in doc/src/sgml/charset.sgml. I made a very minor update. Do you have something more specific in mind? This all looks good to me.
Re: Check whether binaries can be released for s390x
Hi Namrata, On 4/4/23 8:56 AM, Namrata Bhave wrote: Hi, We are looking forward to get help from community on publishing s390x binaries. As per downloads page, apt repo supports Ubuntu on amd,arm,i386 and ppc64le. We had reached out earlier and are ready to provide infra if needed. Wanted to check again if community is willing to help. It'd be better to discuss on pgsql-...@lists.postgresql.org -- that's the mailing list for the website. Thanks, Jonathan OpenPGP_signature Description: OpenPGP digital signature
Re: generic plans and "initial" pruning
On Tue, Apr 4, 2023 at 6:41 AM Tom Lane wrote: > Amit Langote writes: > > [ v38 patchset ] > > I spent a little bit of time looking through this, and concluded that > it's not something I will be wanting to push into v16 at this stage. > The patch doesn't seem very close to being committable on its own > terms, and even if it was now is not a great time in the dev cycle > to be making significant executor API changes. Too much risk of > having to thrash the API during beta, or even change it some more > in v17. I suggest that we push this forward to the next CF with the > hope of landing it early in v17. OK, thanks a lot for your feedback. > A few concrete thoughts: > > * I understand that your plan now is to acquire locks on all the > originally-named tables, then do permissions checks (which will > involve only those tables), then dynamically lock just inheritance and > partitioning child tables as we descend the plan tree. Actually, with the current implementation of the patch, *all* of the relations mentioned in the plan tree would get locked during the ExecInitNode() traversal of the plan tree (and of those in plannedstmt->subplans), not just the inheritance child tables. Locking of non-child tables done by the executor after this patch is duplicative with AcquirePlannerLocks(), so that's something to be improved. > That seems > more or less okay to me, but it could be reflected better in the > structure of the patch perhaps. > > * In particular I don't much like the "viewRelations" list, which > seems like a wart; those ought to be handled more nearly the same way > as other RTEs. (One concrete reason why is that this scheme is going > to result in locking views in a different order than they were locked > during original parsing, which perhaps could contribute to deadlocks.) > Maybe we should store an integer list of which RTIs need to be locked > in the early phase? Building that in the parser/rewriter would provide > a solid guide to the original locking order, so we'd be trivially sure > of duplicating that. (It might be close enough to follow the RT list > order, which is basically what AcquireExecutorLocks does today, but > this'd be more certain to do the right thing.) I'm less concerned > about lock order for child tables because those are just going to > follow the inheritance or partitioning structure. What you've described here sounds somewhat like what I had implemented in the patch versions till v31, though it used a bitmapset named minLockRelids that is initialized by setrefs.c. Your idea of initializing a list before planning seems more appealing offhand than the code I had added in setrefs.c to populate that minLockRelids bitmapset, which would be bms_add_range(1, list_lenth(finalrtable)), followed by bms_del_members(set-of-child-rel-rtis). I'll give your idea a try. > * I don't understand the need for changes like this: > > /* clean up tuple table */ > - ExecClearTuple(node->ps.ps_ResultTupleSlot); > + if (node->ps.ps_ResultTupleSlot) > + ExecClearTuple(node->ps.ps_ResultTupleSlot); > > ISTM that the process ought to involve taking a lock (if needed) > before we have built any execution state for a given plan node, > and if we find we have to fail, returning NULL instead of a > partially-valid planstate node. Otherwise, considerations of how > to handle partially-valid nodes are going to metastasize into all > sorts of places, almost certainly including EXPLAIN for instance. > I think we ought to be able to limit the damage to "parent nodes > might have NULL child links that you wouldn't have expected". > That wouldn't faze ExecEndNode at all, nor most other code. Hmm, yes, taking a lock before allocating any of the stuff to add into the planstate seems like it's much easier to reason about than the alternative I've implemented. > * More attention is needed to comments. For example, in a couple of > places in plancache.c you have removed function header comments > defining API details and not replaced them with any info about the new > details, despite the fact that those details are more complex than the > old. OK, yeah, maybe I've added a bunch of explanations in execMain.c that should perhaps have been in plancache.c. > > It seems I hadn't noted in the ExecEndNode()'s comment that all node > > types' recursive subroutines need to handle the change made by this > > patch that the corresponding ExecInitNode() subroutine may now return > > early without having initialized all state struct fields. > > Also noted in the documentation for CustomScan and ForeignScan that > > the Begin*Scan callback may not have been called at all, so the > > End*Scan should handle that gracefully. > > Yeah, I think we need to avoid adding such requirements. It's the > sort of thing that would far too easily get past developer testing > and only fail once in a blue moon in the field. OK, got it. -- Thanks, Amit Langote EDB: http://www.enterprised
RE: Check whether binaries can be released for s390x
Hi Jonathan, Thank you for getting back. The request is mainly for the developer community to build and publish s390x binaries, apologies if I wasn't clear earlier. We can provide s390x VMs to build and test postgresql binaries if you feel infra is a blocker. Please let me know if any more information is needed. Thank you, Namrata -Original Message- From: Jonathan S. Katz Sent: Tuesday, April 4, 2023 6:53 PM To: Namrata Bhave ; pgsql-hackers@lists.postgresql.org Cc: Vibhuti Sawant Subject: [EXTERNAL] Re: Check whether binaries can be released for s390x Hi Namrata, On 4/4/23 8:56 AM, Namrata Bhave wrote: > Hi, > > We are looking forward to get help from community on publishing s390x > binaries. As per downloads page, apt repo supports Ubuntu on > amd,arm,i386 and ppc64le. > > We had reached out earlier and are ready to provide infra if needed. > Wanted to check again if community is willing to help. It'd be better to discuss on pgsql-...@lists.postgresql.org -- that's the mailing list for the website. Thanks, Jonathan
Re: Should vacuum process config file reload more often
> On 4 Apr 2023, at 00:35, Melanie Plageman wrote: > > On Mon, Apr 3, 2023 at 3:08 PM Andres Freund wrote: >> On 2023-04-03 14:43:14 -0400, Tom Lane wrote: >>> Melanie Plageman writes: v13 attached with requested updates. >>> >>> I'm afraid I'd not been paying any attention to this discussion, >>> but better late than never. I'm okay with letting autovacuum >>> processes reload config files more often than now. However, >>> I object to allowing ProcessConfigFile to be called from within >>> commands in a normal user backend. The existing semantics are >>> that user backends respond to SIGHUP only at the start of processing >>> a user command, and I'm uncomfortable with suddenly deciding that >>> that can work differently if the command happens to be VACUUM. >>> It seems unprincipled and perhaps actively unsafe. >> >> I think it should be ok in commands like VACUUM that already internally start >> their own transactions, and thus require to be run outside of a transaction >> and at the toplevel. I share your concerns about allowing config reload in >> arbitrary places. While we might want to go there, it would require a lot >> more >> analysis. Thinking more on this I'm leaning towards going with allowing more frequent reloads in autovacuum, and saving the same for VACUUM for more careful study. The general case is probably fine but I'm not convinced that there aren't error cases which can present unpleasant scenarios. Regarding the autovacuum part of this patch I think we are down to the final details and I think it's doable to finish this in time for 16. > As an alternative for your consideration, attached v14 set implements > the config file reload for autovacuum only (in 0003) and then enables it > for VACUUM and ANALYZE not in a nested transaction command (in 0004). > > Previously I had the commits in the reverse order for ease of review (to > separate changes to worker limit balancing logic from config reload > code). A few comments on top of already submitted reviews, will do another pass over this later today. + * VacuumFailsafeActive is a defined as a global so that we can determine + * whether or not to re-enable cost-based vacuum delay when vacuuming a table. This comment should be expanded to document who we expect to inspect this variable in order to decide on cost-based vacuum. Moving the failsafe switch into a global context means we face the risk of an extension changing it independently of the GUCs that control it (or the code relying on it) such that these are out of sync. External code messing up internal state is not new and of course outside of our control, but it's worth at least considering. There isn't too much we can do here, but perhaps expand this comment to include a "do not change this" note? +extern bool VacuumFailsafeActive; While I agree with upthread review comments that extensions shoulnd't poke at this, not decorating it with PGDLLEXPORT adds little protection and only cause inconsistencies in symbol exports across platforms. We only explicitly hide symbols in shared libraries IIRC. +extern int VacuumCostLimit; +extern double VacuumCostDelay; ... -extern PGDLLIMPORT int VacuumCostLimit; -extern PGDLLIMPORT double VacuumCostDelay; Same with these, I don't think this is according to our default visibility. Moreover, I'm not sure it's a good idea to perform this rename. This will keep VacuumCostLimit and VacuumCostDelay exported, but change their meaning. Any external code referring to these thinking they are backing the GUCs will still compile, but may be broken in subtle ways. Is there a reason for not keeping the current GUC variables and instead add net new ones? + * TODO: should VacuumCostLimit and VacuumCostDelay be initialized to valid or + * invalid values? + */ +intVacuumCostLimit = 0; +double VacuumCostDelay = -1; I think the important part is to make sure they are never accessed without VacuumUpdateCosts having been called first. I think that's the case here, but it's not entirely clear. Do you see a codepath where that could happen? If they are initialized to a sentinel value we also need to check for that, so initializing to the defaults from the corresponding GUCs seems better. +* Update VacuumCostLimit with the correct value for an autovacuum worker, given Trivial whitespace error in function comment. +static double av_relopt_cost_delay = -1; +static int av_relopt_cost_limit = 0; These need a comment IMO, ideally one that explain why they are initialized to those values. + /* There is at least 1 autovac worker (this worker). */ + Assert(nworkers_for_balance > 0); Is there a scenario where this is expected to fail? If so I think this should be handled and not just an Assert. -- Daniel Gustafsson
Re: Minimal logical decoding on standbys
On Tue, Apr 4, 2023 at 6:05 PM Drouvot, Bertrand
wrote:
>
> On 4/4/23 1:43 PM, Amit Kapila wrote:
> > On Tue, Apr 4, 2023 at 3:14 PM Drouvot, Bertrand
> > wrote:
> >>
> >
> >
> > +static inline bool
> > +LogicalReplicationSlotXidsConflict(ReplicationSlot *s, TransactionId xid)
> > +{
> > + TransactionId slot_xmin;
> > + TransactionId slot_catalog_xmin;
> > +
> > + slot_xmin = s->data.xmin;
> > + slot_catalog_xmin = s->data.catalog_xmin;
> > +
> > + return (((TransactionIdIsValid(slot_xmin) &&
> > TransactionIdPrecedesOrEquals(slot_xmin, xid)) ||
> >
> > For logical slots, slot->data.xmin will always be an
> > InvalidTransactionId. It will only be set/updated for physical slots.
> > So, it is not clear to me why in this and other related functions, you
> > are referring to and or invalidating it.
> >
>
> I think you're right that invalidating/checking only on the catalog xmin is
> enough for logical slot (I'm not sure how I ended up taking the xmin into
> account but
> that seems useless indeed).
>
I think we might want to consider slot's effective_xmin instead of
data.xmin as we use that to store xmin_horizon when we build the full
snapshot.
--
With Regards,
Amit Kapila.
Re: Why enable_hashjoin Completely disables HashJoin
On Tue, Apr 4, 2023 at 3:38 AM Quan Zongliang wrote: > Because someone noticed that when he set enable_hashjoin, > enable_mergejoin and enable_nestloop to off. The statement seemed to get > stuck (actually because it chose the NestedLoop path, which took a long > long time to run). > If enable_hashjoin and enable_nestloop disable generating these two > paths. Then enable_nestloop should do the same thing, but it doesn't. This all seems like expected behavior. If you disable an entire plan type, you should expect to get some bad plans. And if you disable all the plan types, you should still expect to get some plan, but maybe an extremely bad one. -- Robert Haas EDB: http://www.enterprisedb.com
Re: possible proposal plpgsql GET DIAGNOSTICS oid = PG_ROUTINE_OID
Pavel Stehule writes: > There is reduced patch + regress tests One more thing: I do not think it's appropriate to allow this in GET STACKED DIAGNOSTICS. That's about reporting the place where an error occurred, not the current location. Eventually it might be interesting to retrieve the OID of the function that contained the error, but that would be a pretty complicated patch and I am not sure it's worth it. In the meantime I think we should just forbid it. If we do that, then the confusion you were concerned about upthread goes away and we could shorten the keyword back down to "pg_routine_oid", which seems like a good thing for our carpal tunnels. Thoughts? regards, tom lane
Re: doc: add missing "id" attributes to extension packaging page
On 29.03.23 18:03, Brar Piening wrote: On 29.03.2023 at 06:52, Hayato Kuroda (Fujitsu) wrote: FYI - my patch is pushed Thanks! Could you please rebase your patch? Done and tested. Patch is attached. I have committed the most recent patch that added some missing IDs. (I also added a missing xreflabel in passing.) I'll look at the XSLT patch next.
Re: [PATCH] Reuse Workers and Replication Slots during Logical Replication
On Sun, 26 Feb 2023 at 19:11, Melanie Plageman wrote: > > This is cool! Thanks for working on this. > I had a chance to review your patchset and I had some thoughts and > questions. It looks like this patch got a pretty solid review from Melanie Plageman in February just before the CF started. It was never set to Waiting on Author but I think that may be the right state for it. -- Gregory Stark As Commitfest Manager
Re: [PATCH] Clarify the behavior of the system when approaching XID wraparound
On Tue, 4 Apr 2023 at 17:08, Aleksander Alekseev
wrote:
>
> Hi,
>
> > The proposed changes are in patchset v5.
>
> Pavel, John, thanks for your feedback.
>
> > I've looked into the patches v4.
> > For 0001:
> > I think long "not accepting commands that generate" is equivalent to
> > more concise "can't generate".
>
> Frankly I don't think this is a good change for this particular CF
> entry and it violates the consistency with multixact.c. Additionally
> the new message is not accurate. The DBMS _can_ generate new XIDs,
> quite a few of them actually. It merely refuses to do so.
>
> > For all:
> > In a errhint's list what _might_ be done I think AND is a little bit
> > better that OR as the word _might_ means that each of the proposals in
> > the list is a probable, not a sure one.
>
> Well, that's debatable... IMO "or" makes a bit more sense since the
> user knows better whether he/she needs to kill a long-running
> transaction, or run VACUUM, or maybe do both. "And" would imply that
> the user needs to do all of it, which is not necessarily true. Since
> originally it was "or" I suggest we keep it as is for now.
>
> All in all I would prefer keeping the focus on the particular problem
> the patch tries to address.
>
> > For 0003:
> > I think double mentioning of Vacuum at each errhist i.e.: "Execute a
> > database-wide VACUUM in that database" and "...or run a manual
> > database-wide VACUUM." are redundant. The advice "Ensure that
> > autovacuum is progressing,..." is also not needed after advice to
> > "Execute a database-wide VACUUM in that database".
> > [...]
>
> > Okay, great. For v4-0003:
> >
> > Each hint mentions vacuum twice, because it's adding the vacuum message at
> > the end, but not removing it from the beginning. The vacuum string should
> > be on its own line, since we will have to modify that for back branches
> > (skip indexes and truncation).
>
> My bad. Fixed.
>
> > I'm also having second thoughts about "Ensure that autovacuum is
> > progressing" in the hint. That might be better in the docs, if we decide to
> > go ahead with adding a specific checklist there.
>
> OK, done.
>
> > In vacuum.c:
> >
> > errhint("Close open transactions soon to avoid wraparound problems.\n"
> > - "You might also need to commit or roll back old prepared transactions, or
> > drop stale replication slots.")));
> > + "You might also need to commit or roll back old prepared transactions,
> > drop stale replication slots, or kill long-running sessions. Ensure that
> > autovacuum is progressing, or run a manual database-wide VACUUM.")));
> >
> > This appears in vacuum_get_cutoffs(), which is called by vacuum and
> > cluster, and the open transactions were already mentioned, so this is not
> > the place for this change.
>
> Fixed.
>
> > v4-0002:
> >
> > - errhint("Stop the postmaster and vacuum that database in single-user
> > mode.\n"
> > + errhint("VACUUM that database.\n"
> >
> > Further in the spirit of consistency, the mulixact path already has
> > "Execute a database-wide VACUUM in that database.\n", and that seems like
> > better wording.
>
> Agree. Fixed.
Alexander,
Ok, nice! I think it could be moved to committer then.
Pavel.
Re: Minimal logical decoding on standbys
Hi,
On 4/4/23 3:43 PM, Amit Kapila wrote:
On Tue, Apr 4, 2023 at 6:05 PM Drouvot, Bertrand
I think we might want to consider slot's effective_xmin instead of
data.xmin as we use that to store xmin_horizon when we build the full
snapshot.
Oh, I did not know about the 'effective_xmin' and was going to rely only on the
catalog xmin.
Reading the comment in the ReplicationSlot struct about the 'effective_xmin' I
do think it makes sense to use it
(instead of data.xmin).
Please find attached v59 doing so.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.comFrom fad28278fa13bf3564c878aba57fb6d1e6623d59 Mon Sep 17 00:00:00 2001
From: bdrouvotAWS
Date: Tue, 7 Feb 2023 14:08:11 +
Subject: [PATCH v59 6/6] Doc changes describing details about logical
decoding.
Author: Andres Freund (in an older version), Amit Khandekar, Bertrand Drouvot
Reviewed-By: Bertrand Drouvot, Andres Freund, Robert Haas, Fabrizio de Royes
Mello
---
doc/src/sgml/logicaldecoding.sgml | 22 ++
1 file changed, 22 insertions(+)
100.0% doc/src/sgml/
diff --git a/doc/src/sgml/logicaldecoding.sgml
b/doc/src/sgml/logicaldecoding.sgml
index 4e912b4bd4..3da254ed1f 100644
--- a/doc/src/sgml/logicaldecoding.sgml
+++ b/doc/src/sgml/logicaldecoding.sgml
@@ -316,6 +316,28 @@ postgres=# select * from
pg_logical_slot_get_changes('regression_slot', NULL, NU
may consume changes from a slot at any given time.
+
+ A logical replication slot can also be created on a hot standby. To
prevent
+ VACUUM from removing required rows from the system
+ catalogs, hot_standby_feedback should be set on the
+ standby. In spite of that, if any required rows get removed, the slot gets
+ invalidated. It's highly recommended to use a physical slot between the
primary
+ and the standby. Otherwise, hot_standby_feedback will work, but only
while the
+ connection is alive (for example a node restart would break it). Existing
+ logical slots on standby also get invalidated if wal_level on primary is
reduced to
+ less than 'logical'.
+
+
+
+ For a logical slot to be created, it builds a historic snapshot, for which
+ information of all the currently running transactions is essential. On
+ primary, this information is available, but on standby, this information
+ has to be obtained from primary. So, slot creation may wait for some
+ activity to happen on the primary. If the primary is idle, creating a
+ logical slot on standby may take a noticeable time. One option to speed it
+ is to call the pg_log_standby_snapshot on the
primary.
+
+
Replication slots persist across crashes and know nothing about the state
--
2.34.1
From 9e038e69816a1b0722c15515dbfbef3310198e39 Mon Sep 17 00:00:00 2001
From: bdrouvotAWS
Date: Tue, 7 Feb 2023 09:04:12 +
Subject: [PATCH v59 5/6] New TAP test for logical decoding on standby.
In addition to the new TAP test, this commit introduces a new
pg_log_standby_snapshot()
function.
The idea is to be able to take a snapshot of running transactions and write this
to WAL without requesting for a (costly) checkpoint.
Author: Craig Ringer (in an older version), Amit Khandekar, Bertrand Drouvot
Reviewed-By: Bertrand Drouvot, Andres Freund, Robert Haas, Fabrizio de Royes
Mello
---
doc/src/sgml/func.sgml| 15 +
src/backend/access/transam/xlogfuncs.c| 32 +
src/backend/catalog/system_functions.sql | 2 +
src/include/catalog/pg_proc.dat | 3 +
src/test/perl/PostgreSQL/Test/Cluster.pm | 37 +
src/test/recovery/meson.build | 1 +
.../t/035_standby_logical_decoding.pl | 705 ++
7 files changed, 795 insertions(+)
3.1% src/backend/
4.0% src/test/perl/PostgreSQL/Test/
89.7% src/test/recovery/t/
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 918a492234..939fb8019f 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -27034,6 +27034,21 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number *
ps.setting::int + :offset
prepared with .
+
+
+
+ pg_log_standby_snapshot
+
+pg_log_standby_snapshot ()
+pg_lsn
+
+
+Take a snapshot of running transactions and write this to WAL without
+having to wait bgwriter or checkpointer to log one. This one is useful
for
+logical decoding on standby for which logical slot creation is hanging
+until such a record is replayed on the standby.
+
+
diff --git a/src/backend/access/transam/xlogfuncs.c
b/src/backend/access/transam/xlogfuncs.c
index c07daa874f..481e9a47da 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -38,6 +38,7 @@
#include "utils/pg_lsn.h"
#inc
Re: doc: add missing "id" attributes to extension packaging page
On 23.03.23 20:08, Brar Piening wrote: I've also attached the (unchanged) make_html_ids_discoverable patch for convenience so this email now contains two patches for postgresql (ending with .postgresql.patch) and one patch for pgweb (ending with .pgweb.patch). Here is my view on this: First of all, it works very nicely and is very useful. Very welcome. The XSLT implementation looks sound to me. It would be a touch better if it had some comments about which parts of the templates were copied from upstream stylesheets and which were changed. There are examples of such commenting in the existing customization layer. Also, avoid introducing whitespace differences during said copying. However, I wonder if this is the right way to approach this. I don't think we should put these link markers directly into the HTML. It feels like this is the wrong layer. For example, if you have CSS turned off, then all these # marks show up by default. It seems to me that the correct way to do this is to hook in some JavaScript that does this transformation directly on the DOM. Then we don't need to carry this presentation detail in the HTML. Moreover, it would avoid tight coupling between the website and the documentation sources. You can produce the exact same DOM, that part seems okay, just do it elsewhere. Was this approach considered? I didn't see it in the thread.
Re: Check whether binaries can be released for s390x
On Tue, Apr 4, 2023 at 9:30 AM Namrata Bhave wrote: > Thank you for getting back. > > The request is mainly for the developer community to build and publish s390x > binaries, apologies if I wasn't clear earlier. > We can provide s390x VMs to build and test postgresql binaries if you feel > infra is a blocker. > > Please let me know if any more information is needed. As Jonathon said, this discussion is not pertinent to this mailing list. Please use the correct mailing list. I'm not sure that anyone is going to want to undertake this effort for free even if you're willing to provide the hardware for free. But your chances will be a lot better if you ask on the correct mailing list. Thanks, -- Robert Haas EDB: http://www.enterprisedb.com
Re: Commitfest 2023-03 starting tomorrow!
Only a few more days remain before feature freeze. We've now crossed over 100 patches committed according to the CF app: Status summary: March 15March 22March 28April 4 Needs review: 152 128 116 96 Waiting on Author: 42 36 30 21 Ready for Committer: 39 32 32 35 Committed: 61 82 94 101 Moved to next CF: 4 15 17 28 Withdrawn: 17 16 18 20 Rejected: 0 5 6 8 Returned with Feedback: 4 5 6 10 Total: 319. Perhaps more importantly we've crossed *under* 100 patches waiting for review. However I tried to do a pass of the Needs Review patches and found that a lot of them were really waiting for comment and seemed to be useful features or bug fixes that already had a significant amount of work put into them and seemed likely to get committed if there was time available to work on them. There seems to be a bit of a mix of either a) patches that just never got any feedback -- in some cases presumably because the patch required special competency in a niche area or b) patches that had active discussion and patches being updated until discussion died out. Presumably because the author either got busy elsewhere or perhaps the discussion seemed unproductive and exhausted them. What I didn't see, that I expected to see, was patches that were just uninteresting to anyone other than the author but that people were just too polite to reject. So I think these patches are actual useful patches that we would want to have but are likely, modulo some bug fixes, to get moved along to the next CF again without any progress this CF: * Remove dead macro exec_subplan_get_plan * pg_rewind WAL deletion pitfall * Avoid hiding shared filesets in pg_ls_tmpdir (pg_ls_* functions for showing metadata ...) * Fix bogus error emitted by pg_recvlogical when interrupted * Lockless queue of waiters based on atomic operations for LWLock * Add sortsupport for range types and btree_gist * Enable jitlink as an alternative jit linker of legacy Rtdyld and add riscv jitting support * basebackup: support zstd long distance matching * Function to log backtrace of postgres processes * More scalable multixacts buffers and locking * COPY FROM enable FORCE_NULL/FORCE_NOT_NULL on all columns * Add semi-join pushdown to postgres_fdw * Skip replicating the tables specified in except table option * Post-special Page Storage TDE support * Direct I/O (developer-only feature) * Improve doc for autovacuum on partitioned tables * An attempt to avoid locally-committed-but-not-replicated-to-standby-transactions in synchronous replication * Check lateral references within PHVs for memoize cache keys * monitoring usage count distribution * New [relation] options engine * nbtree performance improvements through specialization on key shape * Fix assertion failure in SnapBuildInitialSnapshot() * Speed up releasing of locks * Improve pg_bsd_indent's handling of multiline initialization expressions * Refactoring postgres_fdw/connection.c * Add pg_stat_session * archive modules loose ends * Fix dsa_free() to re-bin segment * Reduce timing overhead of EXPLAIN ANALYZE using rdtsc * clean up permission checks after 599b33b94 * Fix the description of GUC "max_locks_per_transaction" and "max_pred_locks_per_transaction" in guc_table.c * some namespace.c refactoring * Add function to_oct * Switching XLog source from archive to streaming when primary available * BRIN - SK_SEARCHARRAY and scan key preprocessing * Reuse Workers and Replication Slots during Logical Replication
Re: RFC: logical publication via inheritance root?
On Mon, Apr 3, 2023 at 8:53 PM Peter Smith wrote: > > FYI, the WIP patch does not seem to apply cleanly anymore using the latest > HEAD. Yes, sorry -- after 062a84442, the architecture needs to change in a way that I'm still working through. I've moved the patch to Waiting on Author while I figure out the rebase. Thanks, --Jacob
Re: Commitfest 2023-03 starting tomorrow!
"Gregory Stark (as CFM)" writes: > However I tried to do a pass of the Needs Review patches and found > that a lot of them were really waiting for comment and seemed to be > useful features or bug fixes that already had a significant amount of > work put into them and seemed likely to get committed if there was > time available to work on them. Yeah, we just don't have enough people ... > So I think these patches are actual useful patches that we would want > to have but are likely, modulo some bug fixes, to get moved along to > the next CF again without any progress this CF: I have comments on a few of these: > * Remove dead macro exec_subplan_get_plan TBH, I'd reject this one as not being worth the trouble. > * monitoring usage count distribution And I'm dubious how many people care about this, either. > * Improve pg_bsd_indent's handling of multiline initialization expressions This is going to go in once the commit fest is done; we're just holding off to avoid creating merge issues during the CF time crunch. > * clean up permission checks after 599b33b94 I believe that the actual bug fixes are in, and what's left is just a test case that people weren't very excited about adding. So maybe this should get closed out as committed. Perhaps we'll get some of the others done by the end of the week. regards, tom lane
Re: Improving btree performance through specializing by key shape, take 2
Hm. The cfbot has a fairly trivial issue with this with a unused variable: 18:36:17.405] In file included from ../../src/include/access/nbtree.h:1184, [18:36:17.405] from verify_nbtree.c:27: [18:36:17.405] verify_nbtree.c: In function ‘palloc_btree_page’: [18:36:17.405] ../../src/include/access/nbtree_spec.h:51:23: error: unused variable ‘__nbts_ctx’ [-Werror=unused-variable] [18:36:17.405] 51 | #define NBTS_CTX_NAME __nbts_ctx [18:36:17.405] | ^~ [18:36:17.405] ../../src/include/access/nbtree_spec.h:54:43: note: in expansion of macro ‘NBTS_CTX_NAME’ [18:36:17.405] 54 | #define NBTS_MAKE_CTX(rel) const NBTS_CTX NBTS_CTX_NAME = _nbt_spec_context(rel) [18:36:17.405] | ^ [18:36:17.405] ../../src/include/access/nbtree_spec.h:264:28: note: in expansion of macro ‘NBTS_MAKE_CTX’ [18:36:17.405] 264 | #define nbts_prep_ctx(rel) NBTS_MAKE_CTX(rel) [18:36:17.405] | ^ [18:36:17.405] verify_nbtree.c:2974:2: note: in expansion of macro ‘nbts_prep_ctx’ [18:36:17.405] 2974 | nbts_prep_ctx(NULL); [18:36:17.405] | ^
Re: logical decoding and replication of sequences, take 2
Fwiw the cfbot seems to have some failing tests with this patch: [19:05:11.398] # Failed test 'initial test data replicated' [19:05:11.398] # at t/030_sequences.pl line 75. [19:05:11.398] # got: '1|0|f' [19:05:11.398] # expected: '132|0|t' [19:05:11.398] [19:05:11.398] # Failed test 'advance sequence in rolled-back transaction' [19:05:11.398] # at t/030_sequences.pl line 98. [19:05:11.398] # got: '1|0|f' [19:05:11.398] # expected: '231|0|t' [19:05:11.398] [19:05:11.398] # Failed test 'create sequence, advance it in rolled-back transaction, but commit the create' [19:05:11.398] # at t/030_sequences.pl line 152. [19:05:11.398] # got: '1|0|f' [19:05:11.398] # expected: '132|0|t' [19:05:11.398] [19:05:11.398] # Failed test 'advance the new sequence in a transaction and roll it back' [19:05:11.398] # at t/030_sequences.pl line 175. [19:05:11.398] # got: '1|0|f' [19:05:11.398] # expected: '231|0|t' [19:05:11.398] [19:05:11.398] # Failed test 'advance sequence in a subtransaction' [19:05:11.398] # at t/030_sequences.pl line 198. [19:05:11.398] # got: '1|0|f' [19:05:11.398] # expected: '330|0|t' [19:05:11.398] # Looks like you failed 5 tests of 6. -- Gregory Stark As Commitfest Manager
Re: Fix the description of GUC "max_locks_per_transaction" and "max_pred_locks_per_transaction" in guc_table.c
Nathan Bossart writes: > On Wed, Feb 22, 2023 at 12:40:07PM +, wangw.f...@fujitsu.com wrote: >> After some rethinking, I think users can easily get exact value according to >> exact formula, and I think using accurate formula can help users adjust >> max_locks_per_transaction or max_predicate_locks_per_transaction if needed. >> So, >> I used the exact formulas in the attached v2 patch. > IMHO this is too verbose. Yeah, it's impossibly verbose. Even the current wording does not fit nicely in pg_settings output. > Perhaps it could be simplified to something like > The shared lock table is sized on the assumption that at most > max_locks_per_transaction objects per eligible process or prepared > transaction will need to be locked at any one time. I like the "per eligible process" wording, at least for guc_tables.c; or maybe it could be "per server process"? That would be more accurate and not much longer than what we have now. I've got mixed emotions about trying to put the exact formulas into the SGML docs either. Space isn't such a constraint there, but I think the info would soon go out of date (indeed, I think the existing wording was once exactly accurate), and I'm not sure it's worth trying to maintain it precisely. One reason that I'm not very excited about this is that in fact the formula seen in the source code is not exact either; it's a lower bound for how much space will be available. That's because we throw in 100K slop at the bottom of the shmem sizing calculation, and a large chunk of that remains available to be eaten by the lock table if necessary. regards, tom lane
Re: running logical replication as the subscription owner
On Mon, Apr 3, 2023 at 10:09 PM Noah Misch wrote: > I gather we agree on what to do for v16, which is good. I have committed the patches. -- Robert Haas EDB: http://www.enterprisedb.com
Re: psql: Add role's membership options to the \du+ command
"David G. Johnston" writes: > I've marked this Ready for Committer. Hmm ... not sure I like the proposed output. The 'a', 'i', 's' annotations are short but they don't have much else to recommend them. On the other hand, there's nearby precedent for single-letter abbreviations in ACL displays. Nobody particularly likes those, though. Also, if we're modeling this on ACLs then the display could be further shortened to "(ais)" or the like. Also, the patch is ignoring i18n issues. I suppose if we stick with said single-letter abbreviations we'd not translate them, but the construction "rolename from rolename" ought to be translatable IMO. Perhaps it'd be enough to allow replacement of "from", but I wonder if the phrase order would need to be different in some languages? regards, tom lane
Re: psql: Add role's membership options to the \du+ command
On Tue, Apr 4, 2023 at 9:13 AM Tom Lane wrote: > "David G. Johnston" writes: > > I've marked this Ready for Committer. > > Hmm ... not sure I like the proposed output. The 'a', 'i', 's' > annotations are short but they don't have much else to recommend them. > On the other hand, there's nearby precedent for single-letter > abbreviations in ACL displays. Nobody particularly likes those, > though. Also, if we're modeling this on ACLs then the display > could be further shortened to "(ais)" or the like. > I am on board with removing the comma and space between the specifiers. My particular issue with the condensed form is readability, especially the lowercase "i". We aren't so desperate for horizontal space here that compaction seems particularly desirable. > > Also, the patch is ignoring i18n issues. Fair point. > I suppose if we stick with > said single-letter abbreviations we'd not translate them, Correct. I don't see this being a huge issue - the abbreviations are the first letter of the various option "keywords" specified in the syntax. > but the > construction "rolename from rolename" ought to be translatable IMO. > Perhaps it'd be enough to allow replacement of "from", but I wonder > if the phrase order would need to be different in some languages? > > Leveraging position and some optional symbols for readability, and sticking with the premise that abbreviations down to the first letter of the relevant syntax keyword is OK: rolename [g: grantor_role] (ais) I don't have any ideas regarding i18n concerns besides avoiding them by not using words...but I'd much prefer "from" and just hope the equivalent in other languages is just as understandable. I'd rather have the above than go and fully try to emulate ACL presentation just to avoid i18n issues. David J.
Re: psql: Add role's membership options to the \du+ command
On Tue, Apr 4, 2023 at 12:13 PM Tom Lane wrote: > Hmm ... not sure I like the proposed output. The 'a', 'i', 's' > annotations are short but they don't have much else to recommend them. Yeah, I don't like that, either. I'm not sure what the right thing to do is here. It's a problem to have new information in the catalogs that you can't view via \d. But displaying that information as a string of characters that will be gibberish to anyone but an expert doesn't necessarily seem like it really solves the problem. However, if we spell out the words, then it gets bulky. But maybe bulky is better than incomprehensible. -- Robert Haas EDB: http://www.enterprisedb.com
Re: proposal: psql: show current user in prompt
Kirk Wolak writes: > Changed status to Ready for Committer. (100% Guessing here...) Basically, I want to reject this on the grounds that it's not useful enough to justify the overhead of marking the "role" GUC as GUC_REPORT. The problems with it not going to work properly with old servers are an additional reason not to like it. But, if I lose the argument and we do commit this, I think it should just print an empty string when dealing with an old server. "ERR02000" is an awful idea, not least because it could be a real role name. BTW, we should probably get rid of the PQuser() fallback in %n (session_username()) as well. It's unlikely that there are still servers in the wild that don't report "session_authorization", but if we did find one then the output is potentially misleading. I'd rather print nothing than something that might not be your actual session authorization setting. regards, tom lane
RE: [Proposal] Add foreign-server health checks infrastructure
Dear Fujii-san, Tom, Thank you for giving a suggestion! PSA new version. > Regarding 0001 patch, on second thought, to me, it seems odd to expose > a function that doesn't have anything to directly do with PostgreSQL > as a libpq function. The function simply calls poll() on the socket > with POLLRDHUP if it is supported. While it is certainly convenient to > have this function, I'm not sure that it fits within the scope of libpq. > Thought? Current style is motivated by Onder [1] and later discussions. I thought it might be useful for other developers, but OK, I can remove changes on libpq modules. Horiguchi-san has suggested [2] that it might be overkill to use WaitEventSet() mechanism, so I kept using poll(). I reused the same naming as previous version because they actually do something Like libpq, but better naming is very welcome. > Regarding 0002 patch, the behavior of postgres_fdw_verify_connection_states() > in regards to multiple connections using different user mappings seems > not well documented. The function seems to return false if either of > those connections has been closed. I did not considered the situation because I have not came up with the situation that only one of connections to the same foreign server is broken. > This behavior means that it's difficult to identify which connection > has been closed when there are multiple ones to the given server. > To make it easier to identify that, it could be helpful to extend > the postgres_fdw_verify_connection_states() function so that it accepts > a unique connection as an input instead of just the server name. > One suggestion is to extend the function so that it accepts > both the server name and the user name used for the connection, > and checks the specified connection. If only the server name is specified, > the function should check all connections to the server and return false > if any of them are closed. This would be helpful since there is typically > only one connection to the server in most cases. Just to confirm, your point "user name" means a local user, right? I made a patch for addressing them. > Additionally, it would be helpful to extend the postgres_fdw_get_connections() > function to also return the user name used for each connection, > as currently there is no straightforward way to identify it. Added, See 0003. IIUC there is no good way to extract user mapping from its OID, so I have added an function to do that and used it. > The function name "postgres_fdw_verify_connection_states" may seem > unnecessarily long to me. A simpler name like > "postgres_fdw_verify_connection" may be enough? Renamed. > The patch may not be ready for commit due to the review comments, > and with the feature freeze approaching in a few days, > it may not be possible to include this feature in v16. It is sad for me, but it is more important for PostgreSQL to add nicer codes. I changed status to "Needs review" again. [1]: https://www.postgresql.org/message-id/CACawEhW19nPfbFpvfke9eidFDxAy%2Bic36wmY0s936T%3DxzxgHog%40mail.gmail.com [2]: https://www.postgresql.org/message-id/20221017.172642.45253962719866795.horikyota.ntt%40gmail.com Best Regards, Hayato Kuroda FUJITSU LIMITED v38-0001-postgres_fdw-add-postgres_fdw_verify_connection-.patch Description: v38-0001-postgres_fdw-add-postgres_fdw_verify_connection-.patch v38-0002-add-test.patch Description: v38-0002-add-test.patch v38-0003-Extend-postgres_fdw_get_connections-to-return-us.patch Description: v38-0003-Extend-postgres_fdw_get_connections-to-return-us.patch
Re: Minimal logical decoding on standbys
Hi,
On 4/4/23 1:21 PM, Alvaro Herrera wrote:
Hi,
On 2023-Apr-03, Andres Freund wrote:
Hm? That's what the _'s do. We build strings in parts in other places too.
No, what _() does is mark each piece for translation separately. But a
translation cannot be done on string pieces, and later have all the
pieces appended together to form a full sentence. Let me show the
"!terminating" case as example and grab some translations for it from
src/backend/po/de.po:
"invalidating" -> "... wird ungültig gemacht" (?)
(if logical) " obsolete replication" -> " obsolete Replikation"
" slot \"%s\" because it conflicts with recovery" -> " Slot \"%s\", weil sie in
Konflikt mit Wiederherstellung steht"
If you just concatenate all the translated phrases together, the
resulting string will make no sense; keep in mind the "obsolete
replication" part may or not may not be there. And there's no way to
make that work: even if you found an ordering of the English parts that
allows you to translate each piece separately and have it make sense for
German, the same won't work for Spanish or Japanese.
You have to give the translator a complete phrase and let them turn into
a complete translated phrases. Building from parts doesn't work. We're
very good at avoiding string building; we have a couple of cases, but
they are *very* minor.
string 1 "invalidating slot \"%s\" because it conflicts with recovery"
string 2 "invalidating obsolete replication slot \"%s\" because it conflicts with
recovery"
Thanks for looking at it and the explanations!
(I'm not clear on why did Bertrand omitted the word "replication" in the
case where the slot is not logical)
It makes more sense to add it, will do thanks!
I think the errdetail() are okay, it's the errmsg() bits that are bogus.
And yes, well caught on having to use errmsg_internal and
errdetail_internal() to avoid double translation.
So, IIUC having something like this would be fine?
"
if (check_on_xid)
{
if (terminating)
appendStringInfo(&err_msg, _("terminating process %d to release replication slot
\"%s\" because it conflicts with recovery"),
pid,
NameStr(slotname));
else
appendStringInfo(&err_msg, _("invalidating replication slot \"%s\"
because it conflicts with recovery"),
NameStr(slotname));
if (TransactionIdIsValid(*xid))
appendStringInfo(&err_detail, _("The slot conflicted with xid horizon
%u."), *xid);
else
appendStringInfo(&err_detail, _("Logical decoding on standby requires
wal_level to be at least logical on the primary server"));
}
else
{
if (terminating)
appendStringInfo(&err_msg, _("terminating process %d to release replication slot
\"%s\""),
pid,
NameStr(slotname));
else
appendStringInfo(&err_msg, _("invalidating obsolete replication slot
\"%s\""),
NameStr(slotname));
appendStringInfo(&err_detail, _("The slot's restart_lsn %X/%X exceeds the
limit by %llu bytes."),
LSN_FORMAT_ARGS(restart_lsn),
(unsigned long long) (oldestLSN - restart_lsn));
hint = true;
}
ereport(LOG,
errmsg_internal("%s", err_msg.data),
errdetail_internal("%s", err_detail.data),
hint ? errhint("You might need to increase
max_slot_wal_keep_size.") : 0);
"
as err_msg is not concatenated anymore (I mean it's just one sentence build one
time)
and this make use of errmsg_internal() and errdetail_internal().
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Re: possible proposal plpgsql GET DIAGNOSTICS oid = PG_ROUTINE_OID
út 4. 4. 2023 v 16:20 odesílatel Tom Lane napsal:
> Pavel Stehule writes:
> > There is reduced patch + regress tests
>
> One more thing: I do not think it's appropriate to allow this in
> GET STACKED DIAGNOSTICS. That's about reporting the place where
> an error occurred, not the current location. Eventually it might
> be interesting to retrieve the OID of the function that contained
> the error, but that would be a pretty complicated patch and I am
> not sure it's worth it. In the meantime I think we should just
> forbid it.
>
> If we do that, then the confusion you were concerned about upthread
> goes away and we could shorten the keyword back down to "pg_routine_oid",
> which seems like a good thing for our carpal tunnels.
>
> Thoughts?
>
has sense
updated patch attached
Regards
Pavel
>
> regards, tom lane
>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 7c8a49fe43..163ca48f31 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -1639,6 +1639,11 @@ GET DIAGNOSTICS integer_var = ROW_COUNT;
line(s) of text describing the current call stack
(see )
+
+ PG_ROUTINE_OID
+ oid
+ oid of the function currently running
+
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index b0a2cac227..1603ba073d 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2475,6 +2475,12 @@ exec_stmt_getdiag(PLpgSQL_execstate *estate, PLpgSQL_stmt_getdiag *stmt)
}
break;
+ case PLPGSQL_GETDIAG_ROUTINE_OID:
+exec_assign_value(estate, var,
+ ObjectIdGetDatum(estate->func->fn_oid),
+ false, OIDOID, -1);
+ break;
+
default:
elog(ERROR, "unrecognized diagnostic item kind: %d",
diag_item->kind);
diff --git a/src/pl/plpgsql/src/pl_funcs.c b/src/pl/plpgsql/src/pl_funcs.c
index 5a6eadccd5..1b0c857b7d 100644
--- a/src/pl/plpgsql/src/pl_funcs.c
+++ b/src/pl/plpgsql/src/pl_funcs.c
@@ -325,6 +325,8 @@ plpgsql_getdiag_kindname(PLpgSQL_getdiag_kind kind)
return "TABLE_NAME";
case PLPGSQL_GETDIAG_SCHEMA_NAME:
return "SCHEMA_NAME";
+ case PLPGSQL_GETDIAG_ROUTINE_OID:
+ return "PG_ROUTINE_OID";
}
return "unknown";
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index edeb72c380..bf2c04852b 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -318,6 +318,7 @@ static void check_raise_parameters(PLpgSQL_stmt_raise *stmt);
%token K_OR
%token K_PERFORM
%token K_PG_CONTEXT
+%token K_PG_ROUTINE_OID
%token K_PG_DATATYPE_NAME
%token K_PG_EXCEPTION_CONTEXT
%token K_PG_EXCEPTION_DETAIL
@@ -1008,6 +1009,7 @@ stmt_getdiag : K_GET getdiag_area_opt K_DIAGNOSTICS getdiag_list ';'
{
/* these fields are disallowed in stacked case */
case PLPGSQL_GETDIAG_ROW_COUNT:
+case PLPGSQL_GETDIAG_ROUTINE_OID:
if (new->is_stacked)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -1123,6 +1125,9 @@ getdiag_item :
else if (tok_is_keyword(tok, &yylval,
K_RETURNED_SQLSTATE, "returned_sqlstate"))
$$ = PLPGSQL_GETDIAG_RETURNED_SQLSTATE;
+ else if (tok_is_keyword(tok, &yylval,
+K_PG_ROUTINE_OID, "pg_routine_oid"))
+ $$ = PLPGSQL_GETDIAG_ROUTINE_OID;
else
yyerror("unrecognized GET DIAGNOSTICS item");
}
@@ -2523,6 +2528,7 @@ unreserved_keyword :
| K_OPEN
| K_OPTION
| K_PERFORM
+| K_PG_ROUTINE_OID
| K_PG_CONTEXT
| K_PG_DATATYPE_NAME
| K_PG_EXCEPTION_CONTEXT
diff --git a/src/pl/plpgsql/src/pl_unreserved_kwlist.h b/src/pl/plpgsql/src/pl_unreserved_kwlist.h
index 466bdc7a20..3e258a6bb9 100644
--- a/src/pl/plpgsql/src/pl_unreserved_kwlist.h
+++ b/src/pl/plpgsql/src/pl_unreserved_kwlist.h
@@ -85,6 +85,7 @@ PG_KEYWORD("pg_datatype_name", K_PG_DATATYPE_NAME)
PG_KEYWORD("pg_exception_context", K_PG_EXCEPTION_CONTEXT)
PG_KEYWORD("pg_exception_detail", K_PG_EXCEPTION_DETAIL)
PG_KEYWORD("pg_exception_hint", K_PG_EXCEPTION_HINT)
+PG_KEYWORD("pg_routine_oid", K_PG_ROUTINE_OID)
PG_KEYWORD("print_strict_params", K_PRINT_STRICT_PARAMS)
PG_KEYWORD("prior", K_PRIOR)
PG_KEYWORD("query", K_QUERY)
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 355c9f678d..7e574b8052 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -157,7 +157,8 @@ typedef enum PLpgSQL_getdiag_kind
PLPGSQL_GETDIAG_DATATYPE_NAME,
PLPGSQL_GETDIAG_MESSAGE_TEXT,
PLPGSQL_GETDIAG_TABLE_NAME,
- PLPGSQL_GETDIAG_SCHEMA_NAME
+ PLPGSQL_GETDIAG_SCHEMA_NAME,
+ PLPGSQL_GETDIAG_ROUTINE_OID
} PLpgSQL_getdiag_kind;
/*
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 2d26be1a81..df01788a3a 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -5798,3 +5798,32 @
Re: psql: Add role's membership options to the \du+ command
Robert Haas writes:
> I'm not sure what the right thing to do is here. It's a problem to
> have new information in the catalogs that you can't view via
> \d. But displaying that information as a string of
> characters that will be gibberish to anyone but an expert doesn't
> necessarily seem like it really solves the problem. However, if we
> spell out the words, then it gets bulky. But maybe bulky is better
> than incomprehensible.
The existing precedent in \du definitely leans towards "bulky":
regression=# \du
List of roles
Role name | Attributes |
Member of
---++---
alice | Cannot login | {bob}
bob | Cannot login | {}
postgres | Superuser, Create role, Create DB, Replication, Bypass RLS | {}
It seems pretty inconsistent to me to treat the role attributes this
way and then economize in the adjacent column.
Another advantage to spelling out the SQL keywords is that it removes
the question of whether we should translate them.
I wonder if, while we're here, we should apply the idea of
joining-with-newlines-not-commas to the attributes column too.
That's another source of inconsistency in the proposed display.
regards, tom lane
Re: proposal: psql: show current user in prompt
út 4. 4. 2023 v 18:42 odesílatel Tom Lane napsal:
> Kirk Wolak writes:
> > Changed status to Ready for Committer. (100% Guessing here...)
>
> Basically, I want to reject this on the grounds that it's not
> useful enough to justify the overhead of marking the "role" GUC
> as GUC_REPORT. The problems with it not going to work properly
> with old servers are an additional reason not to like it.
>
If I understand to next comment correctly, the overhead should not be too
big
/*
* ReportChangedGUCOptions: report recently-changed GUC_REPORT variables
*
* This is called just before we wait for a new client query.
*
* By handling things this way, we ensure that a ParameterStatus message
* is sent at most once per variable per query, even if the variable
* changed multiple times within the query. That's quite possible when
* using features such as function SET clauses. Function SET clauses
* also tend to cause values to change intraquery but eventually revert
* to their prevailing values; ReportGUCOption is responsible for avoiding
* redundant reports in such cases.
*/
>
> But, if I lose the argument and we do commit this, I think it
> should just print an empty string when dealing with an old server.
> "ERR02000" is an awful idea, not least because it could be a
> real role name.
>
ok
>
> BTW, we should probably get rid of the PQuser() fallback in
> %n (session_username()) as well. It's unlikely that there are
> still servers in the wild that don't report "session_authorization",
> but if we did find one then the output is potentially misleading.
> I'd rather print nothing than something that might not be your
> actual session authorization setting.
>
It should be a separate patch?
Updated patch attached
Regards
Pavel
>
> regards, tom lane
>
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 9f72dd29d8..966cce9559 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2482,6 +2482,7 @@ const char *PQparameterStatus(const PGconn *conn, const char *paramName);
in_hot_standby,
is_superuser,
session_authorization,
+ role,
DateStyle,
IntervalStyle,
TimeZone,
@@ -2496,7 +2497,8 @@ const char *PQparameterStatus(const PGconn *conn, const char *paramName);
9.0;
default_transaction_read_only and
in_hot_standby were not reported by releases before
- 14.)
+ 14;
+ role was not reported by releases before 16;)
Note that
server_version,
server_encoding and
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 29bbec2188..330c085329 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4540,7 +4540,26 @@ testdb=> INSERT INTO my_table VALUES (:'content');
The port number at which the database server is listening.
-
+
+%N
+
+
+ The database role name. This value is specified by command
+ SET ROLE. Until execution of this command
+ the value is none. The value is same like
+ the value of the parameter role (can be
+ displayed by SHOW.
+
+
+
+ This substitution requires PostgreSQL
+ version 15 and up. When you use older version, the empty string
+ is used instead.
+
+
+
+
+
%n
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 8062589efd..3eec4768b3 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4174,7 +4174,7 @@ struct config_string ConfigureNamesString[] =
{"role", PGC_USERSET, UNGROUPED,
gettext_noop("Sets the current role."),
NULL,
- GUC_IS_NAME | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_NOT_WHILE_SEC_REST
+ GUC_IS_NAME | GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_NOT_WHILE_SEC_REST
},
&role_string,
"none",
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e..a304fe1868 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -165,6 +165,35 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
if (pset.db)
strlcpy(buf, session_username(), sizeof(buf));
break;
+ /* DB server user role */
+case 'N':
+ if (pset.db)
+ {
+ int minServerMajor;
+ int serverMajor;
+ const char *val;
+
+ /*
+ * This feature requires GUC "role" to be marked
+ * as GUC_REPORT. Without it is hard to specify fallback
+ * result. Returning empty value can be messy, returning
+ * PQuser like session_username can be messy too.
+ * Exec query is not too practical too, because it doesn't
+ * work when session is not in transactional state, and
+ * CUR
Re: psql: Add role's membership options to the \du+ command
On Tue, Apr 4, 2023 at 1:12 PM Tom Lane wrote: > I wonder if, while we're here, we should apply the idea of > joining-with-newlines-not-commas to the attributes column too. > That's another source of inconsistency in the proposed display. That would make the column narrower, which might be good, because it seems to me that listing the memberships could require quite a lot of space, both vertical and horizontal. There can be any number of memberships, and each of those memberships has a grantor and three flag bits (INHERIT, SET, ADMIN). If some user with a long username has been granted membership with all three of those flags by a grantor who also has a long username, and if we show all that information, we're going to use up a lot of horizontal space. And if there are many such grants, also a lot of vertical space. -- Robert Haas EDB: http://www.enterprisedb.com
Re: possible proposal plpgsql GET DIAGNOSTICS oid = PG_ROUTINE_OID
Pavel Stehule writes: > út 4. 4. 2023 v 16:20 odesílatel Tom Lane napsal: >> If we do that, then the confusion you were concerned about upthread >> goes away and we could shorten the keyword back down to "pg_routine_oid", >> which seems like a good thing for our carpal tunnels. > has sense OK, pushed like that with some cosmetic adjustments (better test case, mostly). regards, tom lane
Re: psql: Add role's membership options to the \du+ command
Robert Haas writes: > On Tue, Apr 4, 2023 at 1:12 PM Tom Lane wrote: >> I wonder if, while we're here, we should apply the idea of >> joining-with-newlines-not-commas to the attributes column too. > That would make the column narrower, which might be good, because it > seems to me that listing the memberships could require quite a lot of > space, both vertical and horizontal. Right, that's what I was thinking. > There can be any number of memberships, and each of those memberships > has a grantor and three flag bits (INHERIT, SET, ADMIN). If some user > with a long username has been granted membership with all three of > those flags by a grantor who also has a long username, and if we show > all that information, we're going to use up a lot of horizontal space. > And if there are many such grants, also a lot of vertical space. Yup --- and if you were incautious enough to not exclude the bootstrap superuser, then you'll also have a very wide Attributes column. We can buy back some of that by joining the attributes with newlines. At some point people are going to have to resort to \x mode for this display, but we should do what we can to put that off as long as we're not sacrificing readability. regards, tom lane
Re: possible proposal plpgsql GET DIAGNOSTICS oid = PG_ROUTINE_OID
út 4. 4. 2023 v 19:34 odesílatel Tom Lane napsal: > Pavel Stehule writes: > > út 4. 4. 2023 v 16:20 odesílatel Tom Lane napsal: > >> If we do that, then the confusion you were concerned about upthread > >> goes away and we could shorten the keyword back down to > "pg_routine_oid", > >> which seems like a good thing for our carpal tunnels. > > > has sense > > OK, pushed like that with some cosmetic adjustments (better test > case, mostly). > Thank you very much Regards Pavel > > regards, tom lane >
Re: Minimal logical decoding on standbys
Hi, On 2023-04-04 13:21:38 +0200, Alvaro Herrera wrote: > On 2023-Apr-03, Andres Freund wrote: > > > Hm? That's what the _'s do. We build strings in parts in other places too. > > No, what _() does is mark each piece for translation separately. But a > translation cannot be done on string pieces, and later have all the > pieces appended together to form a full sentence. Let me show the > "!terminating" case as example and grab some translations for it from > src/backend/po/de.po: > > "invalidating" -> "... wird ungültig gemacht" (?) > > (if logical) " obsolete replication" -> " obsolete Replikation" > > " slot \"%s\" because it conflicts with recovery" -> " Slot \"%s\", weil sie > in Konflikt mit Wiederherstellung steht" > > If you just concatenate all the translated phrases together, the > resulting string will make no sense; keep in mind the "obsolete > replication" part may or not may not be there. And there's no way to > make that work: even if you found an ordering of the English parts that > allows you to translate each piece separately and have it make sense for > German, the same won't work for Spanish or Japanese. Ah, I misunderstood the angle you're coming from. Yes, the pieces need to be reasonable fragments, instead of half-sentences. Greetings, Andres Freund
Re: Option to not use ringbuffer in VACUUM, using it in failsafe mode
On Mon, Apr 3, 2023 at 8:37 PM David Rowley wrote: > > On Tue, 4 Apr 2023 at 02:49, Melanie Plageman > wrote: > > v9 attached. > > I've made a pass on the v9-0001 patch only. Here's what I noted down: Thanks for the review! Attached v10 addresses the review feedback below. Remaining TODOs: - tests - do something about config reload changing GUC > v9-0001: > > 1. In the documentation and comments, generally we always double-space > after a period. I see quite often you're not following this. I've gone through and done this. I noticed after building the docs that it doesn't seem to affect how many spaces are after a period in the rendered docs, but I suppose it affects readability when editing the sgml files. > 2. Doc: We could generally seem to break tags within paragraphs into > multiple lines. You're doing that quite a bit, e.g: > > linkend="glossary-buffer-access-strategy">Buffer Access > Strategy. 0 will disable use of a I've updated all of the ones I could find that I did this with. > 2. This is not a command > > BUFFER_USAGE_LIMIT parameter. > > is probably what you want. I have gone through and attempted to correct all option/command/application tag usages. > 3. I'm not sure I agree that it's a good idea to refer to the strategy > with multiple different names. Here you've called it a "ring buffer", > but in the next sentence, you're calling it a Buffer Access Strategy. > > Specifies the ring buffer size for VACUUM. This size > is used to calculate the number of shared buffers which will be reused > as > part of a Buffer > Access Strategy. 0 disables use of a I've updated this to always prefix any use of ring with "Buffer Access Strategy". I don't know how you'll feel about it. It felt awkward in some places to use Buffer Access Strategy as a complete stand-in for ring buffer. > 4. Can you explain your choice in not just making < 128 a hard error > rather than clamping? > > I guess it means checks like this are made more simple, but that does > not seem like a good enough reason: > > /* check for out-of-bounds */ > if (result < -1 || result > MAX_BAS_RING_SIZE_KB) > > postgres=# vacuum (parallel -1) pg_class; > ERROR: parallel workers for vacuum must be between 0 and 1024 > > Maybe the above is a good guide to follow. > > To allow you to get rid of the clamping code, you'd likely need an > assign hook function for vacuum_buffer_usage_limit. I've added a check hook and replicated the same restrictions in ExecVacuum() where it parses the limit. I have included enforcement of the conditional limit that the ring cannot occupy more than 1/8 of shared buffers. The immediate consequence of this was that my tests were no longer stable (except for the integer overflow one). I have removed them for now until I can come up with a better testing strategy. On the topic of testing, I also thought we should remove the VACUUM(BUFFER_USAGE_LIMIT X, PARALLEL X) test. Though the parallel workers do make their own strategy rings and such a test would be covering some code, I am hesitant to write a test that would never really fail. The observable behavior of not using a strategy will be either 1) basically nothing or 2) the same for parallel and non-parallel. What do you think? > 5. I see vacuum.sgml is full of inconsistencies around the use of > vs . I was going to complain about your: > > ONLY_DATABASE_STATS option. If > ANALYZE is also specified, the > BUFFER_USAGE_LIMIT value is used for both the vacuum > > but I see you've likely just copied what's nearby. > > There are also plenty of usages of in that file. I'd rather > see you use . Maybe there can be some other patch that sweeps > the entire docs to look for OPTION_NAME and > replaces them to use . I haven't done the separate sweep patch, but I have updated my own usages in this set. > 6. I was surprised to see you've added both > GetAccessStrategyWithSize() and GetAccessStrategyWithNBuffers(). I > think the former is suitable for both. GetAccessStrategyWithNBuffers() > seems to be just used once outside of freelist.c This has been updated and reorganized. > 7. I don't think bas_nbuffers() is a good name for an external > function. StrategyGetBufferCount() seems better. I've used this name. > 8. I don't quite follow this comment: > > /* > * TODO: should this be 0 so that we are sure that vacuum() never > * allocates a new bstrategy for us, even if we pass in NULL for that > * parameter? maybe could change how failsafe NULLs out bstrategy if > * so? > */ > > Can you explain under what circumstances would vacuum() allocate a > bstrategy when do_autovacuum() would not? Is this a case of a config > reload where someone changes vacuum_buffer_usage_limit from 0 to > something non-zero? If so, perhaps do_autovacuum() needs to detect > this and allocate a strategy rather than having vacuum() do it once > per table (wastefully). Hmm. Yes, I started hacking on this, but I t
Re: Minimal logical decoding on standbys
Hi,
On 2023-04-04 18:54:33 +0200, Drouvot, Bertrand wrote:
> if (check_on_xid)
> {
> if (terminating)
> appendStringInfo(&err_msg, _("terminating process %d to release
> replication slot \"%s\" because it conflicts with recovery"),
> pid,
> NameStr(slotname));
FWIW, I would just use exactly the same error message as today here.
errmsg("terminating process %d
to release replication slot \"%s\"",
active_pid,
NameStr(slotname)),
This is accurate for both the existing and the new case. Then there's no need
to put that string into a stringinfo either.
Greetings,
Andres Freund
Re: proposal: psql: show current user in prompt
Pavel Stehule writes: > út 4. 4. 2023 v 18:42 odesílatel Tom Lane napsal: >> Basically, I want to reject this on the grounds that it's not >> useful enough to justify the overhead of marking the "role" GUC >> as GUC_REPORT. The problems with it not going to work properly >> with old servers are an additional reason not to like it. > If I understand to next comment correctly, the overhead should not be too > big Yeah, but how big is the use-case? The reason I'm skeptical is that half the time what you're going to get is "none": $ psql psql (16devel) Type "help" for help. regression=# show role; role -- none (1 row) That's required by SQL spec I believe, but that doesn't make it useful data to keep in one's prompt. regards, tom lane
Re: Remove dead macro exec_subplan_get_plan
On Fri, 16 Sept 2022 at 03:33, Zhang Mingli wrote: > > On Sep 16, 2022, 14:47 +0800, Richard Guo , wrote: > > How about add it to the CF to not lose track of it? > > Will add it, thanks~ I guess not losing track of it is only helpful if we do eventually commit it. Otherwise we would rather lose track of it :) I think the conclusion here was that the actual list is still used and cleaning up unused macros isn't worth the hassle unless it's part of some larger patch? I mean, it doesn't seem like a lot of hassle but nobody seems to have been interested in pursuing it since 2022 so I guess it's not going to happen. I don't want to keep moving patches forward release to release that nobody's interested in committing. So I'm going to mark this one rejected for now. We can always update that if it comes up again. -- Gregory Stark As Commitfest Manager
Re: Minimal logical decoding on standbys
On Tue, 2023-04-04 at 11:42 +0200, Drouvot, Bertrand wrote: > Done in V58 and now this is as simple as: Minor comments on 0004 (address if you agree): * Consider static inline for WalSndWakeupProcessRequests()? * Is the WalSndWakeup() in KeepFileRestoredFromArchive() more like the flush case? Why is the second argument unconditionally true? I don't think the cascading logical walsenders have anything to do until the WAL is actually applied. Otherwise, looks good! Regards, Jeff Davis
Re: monitoring usage count distribution
On Mon, 30 Jan 2023 at 18:31, Nathan Bossart wrote: > > My colleague Jeremy Schneider (CC'd) was recently looking into usage count > distributions for various workloads, and he mentioned that it would be nice > to have an easy way to do $SUBJECT. I've attached a patch that adds a > pg_buffercache_usage_counts() function. This function returns a row per > possible usage count with some basic information about the corresponding > buffers. > > postgres=# SELECT * FROM pg_buffercache_usage_counts(); > usage_count | buffers | dirty | pinned > -+-+---+ >0 | 0 | 0 | 0 >1 |1436 | 671 | 0 >2 | 102 |88 | 0 >3 | 23 |21 | 0 >4 | 9 | 7 | 0 >5 | 164 | 106 | 0 > (6 rows) > > This new function provides essentially the same information as > pg_buffercache_summary(), but pg_buffercache_summary() only shows the > average usage count for the buffers in use. If there is interest in this > idea, another approach to consider could be to alter > pg_buffercache_summary() instead. Tom expressed skepticism that there's wide interest here. It seems as much from the lack of response. But perhaps that's just because people don't understand what the importance of this info is -- I certainly don't :) I feel like the original sin here is having the function return an aggregate data. If it returned the raw data then people could slice, dice, and aggregate the data in any ways they want using SQL. And perhaps people would come up with queries that have more readily interpretable important information? Obviously there are performance questions in that but I suspect they might be solvable given how small the data for each buffer are. Just as a warning though -- if nobody was interested in this patch please don't take my comments as a recommendation that you spend a lot of time developing a more complex version in the same direction without seeing if anyone agrees with my suggestion :) -- greg
Re: monitoring usage count distribution
On Mon, Jan 30, 2023 at 6:30 PM Nathan Bossart wrote: > My colleague Jeremy Schneider (CC'd) was recently looking into usage count > distributions for various workloads, and he mentioned that it would be nice > to have an easy way to do $SUBJECT. I've attached a patch that adds a > pg_buffercache_usage_counts() function. This function returns a row per > possible usage count with some basic information about the corresponding > buffers. > > postgres=# SELECT * FROM pg_buffercache_usage_counts(); > usage_count | buffers | dirty | pinned > -+-+---+ >0 | 0 | 0 | 0 >1 |1436 | 671 | 0 >2 | 102 |88 | 0 >3 | 23 |21 | 0 >4 | 9 | 7 | 0 >5 | 164 | 106 | 0 > (6 rows) > > This new function provides essentially the same information as > pg_buffercache_summary(), but pg_buffercache_summary() only shows the > average usage count for the buffers in use. If there is interest in this > idea, another approach to consider could be to alter > pg_buffercache_summary() instead. I'm skeptical that pg_buffercache_summary() is a good idea at all, but having it display the average usage count seems like a particularly poor idea. That information is almost meaningless. Replacing that with a six-element integer array would be a clear improvement and, IMHO, better than adding yet another function to the extension. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Commitfest 2023-03 starting tomorrow!
On Tue, 4 Apr 2023 at 11:18, Tom Lane wrote: > > > * clean up permission checks after 599b33b94 > > I believe that the actual bug fixes are in, and what's left is just a test > case that people weren't very excited about adding. So maybe this should > get closed out as committed. I'm not super convinced about this one. I'm not a big "all tests are good tests" believer but this test seems like a pretty reasonable one. Permissions checks and user mappings are user-visible behaviour that are easy to overlook when making changes with unexpected side effects. It seems like the test would be just as easy to commit as to not commit and I don't see anything tricky about it that would necessitate a more in depth review. -- greg
Re: monitoring usage count distribution
Robert Haas writes: > On Mon, Jan 30, 2023 at 6:30 PM Nathan Bossart > wrote: >> My colleague Jeremy Schneider (CC'd) was recently looking into usage count >> distributions for various workloads, and he mentioned that it would be nice >> to have an easy way to do $SUBJECT. > I'm skeptical that pg_buffercache_summary() is a good idea at all, but > having it display the average usage count seems like a particularly > poor idea. That information is almost meaningless. Replacing that with > a six-element integer array would be a clear improvement and, IMHO, > better than adding yet another function to the extension. I had not realized that pg_buffercache_summary() is new in v16, but since it is, we still have time to rethink its definition. +1 for de-aggregating --- I agree that the overall average is unlikely to have much value. regards, tom lane
Re: proposal: psql: show current user in prompt
út 4. 4. 2023 v 19:55 odesílatel Tom Lane napsal: > Pavel Stehule writes: > > út 4. 4. 2023 v 18:42 odesílatel Tom Lane napsal: > >> Basically, I want to reject this on the grounds that it's not > >> useful enough to justify the overhead of marking the "role" GUC > >> as GUC_REPORT. The problems with it not going to work properly > >> with old servers are an additional reason not to like it. > > > If I understand to next comment correctly, the overhead should not be too > > big > > Yeah, but how big is the use-case? The reason I'm skeptical is that > half the time what you're going to get is "none": > > $ psql > psql (16devel) > Type "help" for help. > > regression=# show role; > role > -- > none > (1 row) > > That's required by SQL spec I believe, but that doesn't make it useful > data to keep in one's prompt. > Who needs it, and who uses different roles, then very quickly uses SET ROLE TO command. But I fully agree so current behavior can be a little bit messy. I like this feature, and I think it can have some benefits. Proposed implementation is minimalistic. One hard problem is translation of the oid of current_user to name. It requires an opened transaction, and then it cannot be postponed to the end of the statement. On the other hand, when the change of role is done inside a nested command, then it should not be visible from the client side. Can you accept the introduction of a new invisible GUC, that can be modified only by SET ROLE TO command when it is executed as top command? Regards Pavel > > regards, tom lane >
fill_seq_fork_with_data() initializes buffer without lock
Hi,
Look at:
static void
fill_seq_fork_with_data(Relation rel, HeapTuple tuple, ForkNumber forkNum)
{
Buffer buf;
Pagepage;
sequence_magic *sm;
OffsetNumber offnum;
/* Initialize first page of relation with special magic number */
buf = ReadBufferExtended(rel, forkNum, P_NEW, RBM_NORMAL, NULL);
Assert(BufferGetBlockNumber(buf) == 0);
page = BufferGetPage(buf);
PageInit(page, BufferGetPageSize(buf), sizeof(sequence_magic));
sm = (sequence_magic *) PageGetSpecialPointer(page);
sm->magic = SEQ_MAGIC;
/* Now insert sequence tuple */
LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
Clearly we are modifying the page (via PageInit()), without holding a buffer
lock, which is only acquired subsequently.
It's clearly unlikely to cause bad consequences - the sequence doesn't yet
really exist, and we haven't seen any reports of a problem - but it doesn't
seem quite impossible that it would cause problems.
As far as I can tell, this goes back to the initial addition of the sequence
code, in e8647c45d66a - I'm too lazy to figure out whether it possibly wasn't
a problem in 1997 for some reason.
Greetings,
Andres Freund
Re: Minimal logical decoding on standbys
On Tue, Apr 4, 2023 at 5:44 AM Drouvot, Bertrand
wrote:
> Oh right, even better, thanks!
> Done in V58 and now this is as simple as:
>
> + if (DoNotInvalidateSlot(s, xid, &oldestLSN))
> {
> /* then, we are not forcing for invalidation */
Thanks for your continued work on $SUBJECT. I just took a look at
0004, and I think that at the very least the commit message needs
work. Nobody who is not a hacker is going to understand what problem
this is fixing, because it makes reference to the names of functions
and structure members rather than user-visible behavior. In fact, I'm
not really sure that I understand the problem myself. It seems like
the problem is that on a standby, WAL senders will get woken up too
early, before we have any WAL to send. That's presumably OK, in the
sense that they'll go back to sleep and eventually wake up again, but
it means they might end up chronically behind sending out WAL to
cascading standbys. If that's right, I think it should be spelled out
more clearly in the commit message, and maybe also in the code
comments.
But the weird thing is that most (all?) of the patch doesn't seem to
be about that issue at all. Instead, it's about separating wakeups of
physical walsenders from wakeups of logical walsenders. I don't see
how that could ever fix the kind of problem I mentioned in the
preceding paragraph, so my guess is that this is a separate change.
But this change doesn't really seem adequately justified. The commit
message says that it "helps to filter what kind of walsender
we want to wakeup based on the code path" but that's awfully vague
about what the actual benefit is. I wonder whether many people have a
mix of physical and logical systems connecting to the same machine
such that this would even help, and if they do have that, would this
really do enough to solve any performance problem that might be caused
by too many wakeups?
--
Robert Haas
EDB: http://www.enterprisedb.com
Re: psql: Add role's membership options to the \du+ command
On Tue, Apr 4, 2023 at 10:37 AM Tom Lane wrote: > Robert Haas writes: > > On Tue, Apr 4, 2023 at 1:12 PM Tom Lane wrote: > >> I wonder if, while we're here, we should apply the idea of > >> joining-with-newlines-not-commas to the attributes column too. > > > That would make the column narrower, which might be good, because it > > seems to me that listing the memberships could require quite a lot of > > space, both vertical and horizontal. > > Right, that's what I was thinking. > > So, by way of example: regress_du_role1 | cannot login | regress_du_role0 granted by regress_du_admin with admin, inherit, set | Description for regress_du_role1 ~140 character width with description No translations, all words are either identical to syntax or identifiers. I'm on board with newlines in the attributes field. The specific member of column changes are: from -> granted by ( ) -> "with" ais -> admin, inherit, set I'm good with any or all of those selections, either as-is or in the more verbose form. David J.
Re: proposal: psql: show current user in prompt
út 4. 4. 2023 v 20:50 odesílatel Pavel Stehule napsal: > > > út 4. 4. 2023 v 19:55 odesílatel Tom Lane napsal: > >> Pavel Stehule writes: >> > út 4. 4. 2023 v 18:42 odesílatel Tom Lane napsal: >> >> Basically, I want to reject this on the grounds that it's not >> >> useful enough to justify the overhead of marking the "role" GUC >> >> as GUC_REPORT. The problems with it not going to work properly >> >> with old servers are an additional reason not to like it. >> >> > If I understand to next comment correctly, the overhead should not be >> too >> > big >> >> Yeah, but how big is the use-case? The reason I'm skeptical is that >> half the time what you're going to get is "none": >> >> $ psql >> psql (16devel) >> Type "help" for help. >> >> regression=# show role; >> role >> -- >> none >> (1 row) >> >> That's required by SQL spec I believe, but that doesn't make it useful >> data to keep in one's prompt. >> > > Who needs it, and who uses different roles, then very quickly uses SET > ROLE TO command. > > But I fully agree so current behavior can be a little bit messy. I like > this feature, and I think it can have some benefits. Proposed > implementation is minimalistic. > > One hard problem is translation of the oid of current_user to name. It > requires an opened transaction, and then it cannot be postponed to the end > of the statement. On the other hand, when the change of role is done inside > a nested command, then it should not be visible from the client side. > > Can you accept the introduction of a new invisible GUC, that can be > modified only by SET ROLE TO command when it is executed as top command? > It was stupid idea. There can be implemented fallbacks. When the role is "none", then the :USER can be displayed instead. It can work, because the custom role "none" is not allowed (2023-04-04 21:10:25) postgres=# create role none; ERROR: role name "none" is reserved LINE 1: create role none; ? > > Regards > > Pavel > > > > > > >> >> regards, tom lane >> >
Re: Parallel Full Hash Join
Thomas Munro writes:
> I committed the main patch.
This left the following code in hash_inner_and_outer (joinpath.c):
/*
* If the joinrel is parallel-safe, we may be able to consider a
* partial hash join. However, we can't handle JOIN_UNIQUE_OUTER,
* because the outer path will be partial, and therefore we won't be
* able to properly guarantee uniqueness. Similarly, we can't handle
* JOIN_FULL and JOIN_RIGHT, because they can produce false null
* extended rows. Also, the resulting path must not be parameterized.
*/
if (joinrel->consider_parallel &&
save_jointype != JOIN_UNIQUE_OUTER &&
outerrel->partial_pathlist != NIL &&
bms_is_empty(joinrel->lateral_relids))
{
The comment is no longer in sync with the code: this if-test used to
reject JOIN_FULL and JOIN_RIGHT, and no longer does so, but the comment
still claims it should. Shouldn't we drop the sentence beginning
"Similarly"? (I see that there's now one sub-section that still rejects
such cases, but it no longer seems correct to claim that they're rejected
overall.)
regards, tom lane
Re: SQL/JSON revisited
Hi hackers!
The latest SQL standard contains dot notation for JSON. Are there any plans
to include it into Pg 16?
Or maybe we should start a separate thread for it?
Thanks!
On Tue, Apr 4, 2023 at 3:36 PM Alvaro Herrera
wrote:
> On 2023-Apr-04, Amit Langote wrote:
>
> > On Tue, Apr 4, 2023 at 2:16 AM Alvaro Herrera
> wrote:
>
> > > - the gram.y solution to the "ON ERROR/ON EMPTY" clauses is quite ugly.
> > > I think we could make that stuff use something similar to
> > > ConstraintAttributeSpec with an accompanying post-processing
> function.
> > > That would reduce the number of ad-hoc hacks, which seem excessive.
> >
> > Do you mean the solution involving the JsonBehavior node?
>
> Right. It has spilled as the separate on_behavior struct in the core
> parser %union in addition to the raw jsbehavior, which is something
> we've gone 30 years without having, and I don't see why we should start
> now.
>
> This stuff is terrible:
>
> json_exists_error_clause_opt:
> json_exists_error_behavior ON ERROR_P { $$ = $1; }
> | /* EMPTY */ { $$ = NULL; }
> ;
>
> json_exists_error_behavior:
> ERROR_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR,
> NULL); }
> | TRUE_P{ $$ = makeJsonBehavior(JSON_BEHAVIOR_TRUE,
> NULL); }
> | FALSE_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_FALSE,
> NULL); }
> | UNKNOWN { $$ = makeJsonBehavior(JSON_BEHAVIOR_UNKNOWN,
> NULL); }
> ;
>
> json_value_behavior:
> NULL_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_NULL, NULL);
> }
> | ERROR_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR,
> NULL); }
> | DEFAULT a_expr{ $$ =
> makeJsonBehavior(JSON_BEHAVIOR_DEFAULT, $2); }
> ;
>
> json_value_on_behavior_clause_opt:
> json_value_behavior ON EMPTY_P
> { $$.on_empty = $1; $$.on_error =
> NULL; }
> | json_value_behavior ON EMPTY_P json_value_behavior ON ERROR_P
> { $$.on_empty = $1; $$.on_error = $4; }
> | json_value_behavior ON ERROR_P
> { $$.on_empty = NULL; $$.on_error =
> $1; }
> | /* EMPTY */
> { $$.on_empty = NULL; $$.on_error =
> NULL; }
> ;
>
> json_query_behavior:
> ERROR_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR,
> NULL); }
> | NULL_P{ $$ = makeJsonBehavior(JSON_BEHAVIOR_NULL,
> NULL); }
> | EMPTY_P ARRAY { $$ =
> makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
> /* non-standard, for Oracle compatibility only */
> | EMPTY_P { $$ =
> makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
> | EMPTY_P OBJECT_P { $$ =
> makeJsonBehavior(JSON_BEHAVIOR_EMPTY_OBJECT, NULL); }
> | DEFAULT a_expr{ $$ =
> makeJsonBehavior(JSON_BEHAVIOR_DEFAULT, $2); }
> ;
>
> json_query_on_behavior_clause_opt:
> json_query_behavior ON EMPTY_P
> { $$.on_empty = $1; $$.on_error =
> NULL; }
> | json_query_behavior ON EMPTY_P json_query_behavior ON ERROR_P
> { $$.on_empty = $1; $$.on_error = $4; }
> | json_query_behavior ON ERROR_P
> { $$.on_empty = NULL; $$.on_error =
> $1; }
> | /* EMPTY */
> { $$.on_empty = NULL; $$.on_error =
> NULL; }
> ;
>
> Surely this can be made cleaner.
>
> By the way -- that comment about clauses being non-standard, can you
> spot exactly *which* clauses that comment applies to?
>
> --
> Álvaro Herrera PostgreSQL Developer —
> https://www.EnterpriseDB.com/
> "El número de instalaciones de UNIX se ha elevado a 10,
> y se espera que este número aumente" (UPM, 1972)
>
>
>
--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/
Re: SQL/JSON revisited
On 4/4/23 3:40 PM, Nikita Malakhov wrote: Hi hackers! The latest SQL standard contains dot notation for JSON. Are there any plans to include it into Pg 16? Or maybe we should start a separate thread for it? I would recommend starting a new thread to discuss the dot notation. Thanks, Jonathan OpenPGP_signature Description: OpenPGP digital signature
Re: doc: add missing "id" attributes to extension packaging page
On 04.04.2023 at 16:54, Peter Eisentraut wrote: First of all, it works very nicely and is very useful. Very welcome. Thank you! The XSLT implementation looks sound to me. It would be a touch better if it had some comments about which parts of the templates were copied from upstream stylesheets and which were changed. There are examples of such commenting in the existing customization layer. Also, avoid introducing whitespace differences during said copying. I will amend the patch if we agree that this is the way forward. However, I wonder if this is the right way to approach this. I don't think we should put these link markers directly into the HTML. It feels like this is the wrong layer. For example, if you have CSS turned off, then all these # marks show up by default. I'd consider this a feature rather than a problem but this is certainly debatable. I cannot reliably predict what expectations a user who is browsing the docs with CSS disabled might have. The opposite is true too. If we'd move the id links feature to javascript, a user who has javascript disabled will not see them. Is this what they'd want? I don't know. Also, while about 1-2% of users have Javascript disabled, I haven't heard of disabling CSS except for debugging purposes. In general I'd consider the fact that CSS or Javascript might be disabled a niche problem that isn't really worth much debating but there is definitely something to consider regarding people using screen readers who might suffer from one or the other behavior and I'd definitely be interested what behavior these users would expect. Would they want to use the id link feature or would the links rather disrupt their reading experience - I have no idea TBH and I hate speculating about other people's preferences. It seems to me that the correct way to do this is to hook in some JavaScript that does this transformation directly on the DOM. Then we don't need to carry this presentation detail in the HTML. Moreover, it would avoid tight coupling between the website and the documentation sources. You can produce the exact same DOM, that part seems okay, just do it elsewhere. Was this approach considered? I didn't see it in the thread. I briefly touched the topic in [1] and [2] but we didin't really follow up on the best approach. Regards, Brar [1] https://www.postgresql.org/message-id/68b9c435-d017-93cc-775a-c604db9ec683%40gmx.de [2] https://www.postgresql.org/message-id/a75b6d7c-3fa4-d6a8-cf23-6b5180237392%40gmx.de
Re: psql: Add role's membership options to the \du+ command
On Tue, Apr 4, 2023 at 3:02 PM David G. Johnston wrote: > So, by way of example: > > regress_du_role1 | cannot login | regress_du_role0 granted by > regress_du_admin with admin, inherit, set | Description for regress_du_role1 > > ~140 character width with description That seems wider than necessary. Why not have the third column be something like regress_du_role0 by regress_du_admin (admin, inherit, set)? -- Robert Haas EDB: http://www.enterprisedb.com
Re: Should vacuum process config file reload more often
On Tue, Apr 4, 2023 at 4:27 AM Masahiko Sawada wrote:
> The 0001 patch mostly looks good to me except for one
> point:
>
> @@ -391,7 +389,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
> Assert(params->index_cleanup != VACOPTVALUE_UNSPECIFIED);
> Assert(params->truncate != VACOPTVALUE_UNSPECIFIED &&
> params->truncate != VACOPTVALUE_AUTO);
> -vacrel->failsafe_active = false;
> +VacuumFailsafeActive = false;
> vacrel->consider_bypass_optimization = true;
> vacrel->do_index_vacuuming = true;
>
> Looking at the 0003 patch, we set VacuumFailsafeActive to false per table:
>
> +/*
> + * Ensure VacuumFailsafeActive has been reset
> before vacuuming the
> + * next relation relation.
> + */
> +VacuumFailsafeActive = false;
>
> Given that we ensure it's reset before vacuuming the next table, do we
> need to reset it in heap_vacuum_rel?
I've changed the one in heap_vacuum_rel() to an assert.
> (there is a typo; s/relation relation/relation/)
Thanks! fixed.
> > > 0002:
> > >
> > > @@ -2388,6 +2398,7 @@ vac_max_items_to_alloc_size(int max_items)
> > > return offsetof(VacDeadItems, items) +
> > > sizeof(ItemPointerData) * max_items;
> > > }
> > >
> > > +
> > > /*
> > > * vac_tid_reaped() -- is a particular tid deletable?
> > > *
> > >
> > > Unnecessary new line. There are some other unnecessary new lines in this
> > > patch.
> >
> > Thanks! I think I got them all.
> >
> > > ---
> > > @@ -307,6 +309,8 @@ extern PGDLLIMPORT pg_atomic_uint32
> > > *VacuumActiveNWorkers;
> > > extern PGDLLIMPORT int VacuumCostBalanceLocal;
> > >
> > > extern bool VacuumFailsafeActive;
> > > +extern int VacuumCostLimit;
> > > +extern double VacuumCostDelay;
> > >
> > > and
> > >
> > > @@ -266,8 +266,6 @@ extern PGDLLIMPORT int
> > > max_parallel_maintenance_workers;
> > > extern PGDLLIMPORT int VacuumCostPageHit;
> > > extern PGDLLIMPORT int VacuumCostPageMiss;
> > > extern PGDLLIMPORT int VacuumCostPageDirty;
> > > -extern PGDLLIMPORT int VacuumCostLimit;
> > > -extern PGDLLIMPORT double VacuumCostDelay;
> > >
> > > Do we need PGDLLIMPORT too?
> >
> > I was on the fence about this. I annotated the new guc variables
> > vacuum_cost_delay and vacuum_cost_limit with PGDLLIMPORT, but I did not
> > annotate the variables used in vacuum code (VacuumCostLimit/Delay). I
> > think whether or not this is the right choice depends on two things:
> > whether or not my understanding of PGDLLIMPORT is correct and, if it is,
> > whether or not we want extensions to be able to access
> > VacuumCostLimit/Delay or if just access to the guc variables is
> > sufficient/desirable.
>
> I guess it would be better to keep both accessible for backward
> compatibility. Extensions are able to access both GUC values and
> values that are actually used for vacuum delays (as we used to use the
> same variables).
> Here are some review comments for 0002-0004 patches:
>
> 0002:
> -if (MyWorkerInfo)
> +if (am_autovacuum_launcher)
> +return;
> +
> +if (am_autovacuum_worker)
> {
> -VacuumCostDelay = MyWorkerInfo->wi_cost_delay;
> VacuumCostLimit = MyWorkerInfo->wi_cost_limit;
> +VacuumCostDelay = MyWorkerInfo->wi_cost_delay;
> +}
>
> Isn't it a bit safer to check MyWorkerInfo instead of
> am_autovacuum_worker?
Ah, since we access it. I've made the change.
> Also, I don't think there is any reason why we want to exclude only
> the autovacuum launcher.
My rationale is that the launcher is the only other process type which
might reasonably be executing this code besides autovac workers, client
backends doing VACUUM/ANALYZE, and parallel vacuum workers. Is it
confusing to have the launcher have VacuumCostLimt and VacuumCostDelay
set to the guc values for explicit VACUUM and ANALYZE -- even if the
launcher doesn't use these variables?
I've removed the check, because I do agree with you that it may be
unnecessarily confusing in the code.
> ---
> + * TODO: should VacuumCostLimit and VacuumCostDelay be initialized to valid
> or
> + * invalid values?
>
> How about using the default value of normal backends, 200 and 0?
I've gone with this suggestion
> 0003:
>
> @@ -83,6 +84,7 @@ int vacuum_cost_limit;
> */
> intVacuumCostLimit = 0;
> double VacuumCostDelay = -1;
> +static bool vacuum_can_reload_config = false;
>
> In vacuum.c, we use snake case for GUC parameters and camel case for
> other global variables, so it seems better to rename it
> VacuumCanReloadConfig. Sorry, that's my fault.
I have renamed it.
> 0004:
>
> +if (tab->at_dobalance)
> +pg_atomic_test_set_flag(&MyWorkerInfo->wi_dobalance);
> +else
>
> The comment of pg_atomic_
Re: SQL/JSON revisited
On 2023-04-04 Tu 08:36, Alvaro Herrera wrote: Surely this can be made cleaner. By the way -- that comment about clauses being non-standard, can you spot exactly *which* clauses that comment applies to? Sadly, I don't think we're going to be able to make further progress before feature freeze. Thanks to Alvaro for advancing us a way down the field. I hope we can get the remainder committed in the July CF. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)
On 03.04.2023 23:50, Melanie Plageman wrote: Attached is a tiny patch to add standalone backend type to pg_stat_activity documentation (referenced by pg_stat_io). I mentioned both the bootstrap process and single user mode process in the docs, though I can't imagine that the bootstrap process is relevant for pg_stat_activity. After a little thought... I'm not sure about the term 'bootstrap process'. I can't find this term in the documentation. Do I understand correctly that this is a postmaster? If so, then the postmaster process is not shown in pg_stat_activity. Perhaps it may be worth adding a description of the standalone backend to pg_stat_io, not to pg_stat_activity. Something like: backend_type is all types from pg_stat_activity plus 'standalone backend', which is used for the postmaster process and in a single user mode. I also noticed that the pg_stat_activity docs call background workers "parallel workers" (though it also mentions that extensions could have other background workers registered), but this seems a bit weird because pg_stat_activity uses GetBackendTypeDesc() and this prints "background worker" for type B_BG_WORKER. Background workers doing parallelism tasks is what users will most often see in pg_stat_activity, but I feel like it is confusing to have it documented as something different than what would appear in the view. Unless I am misunderstanding something... 'parallel worker' appears in the pg_stat_activity for parallel queries. I think it's right here. -- Pavel Luzanov Postgres Professional: https://postgrespro.com
Re: psql: Add role's membership options to the \du+ command
On 04.04.2023 23:00, Robert Haas wrote: On Tue, Apr 4, 2023 at 3:02 PM David G. Johnston wrote: So, by way of example: regress_du_role1 | cannot login | regress_du_role0 granted by regress_du_admin with admin, inherit, set | Description for regress_du_role1 That seems wider than necessary. Why not have the third column be something like regress_du_role0 by regress_du_admin (admin, inherit, set)? 'granted by' can be left without translation, but just 'by' required translation, as I think. -- Pavel Luzanov Postgres Professional: https://postgrespro.com
Re: ICU locale validation / canonicalization
MSVC now says this on master: [17:48:12.446] c:\cirrus\src\backend\utils\adt\pg_locale.c(2912) : warning C4715: 'icu_language_tag': not all control paths return a value CI doesn't currently fail for MSVC warnings, so it's a bit hidden. FWIW cfbot does show this with a ⚠ sign with its new system for grovelling through logs, which will now show up on every entry now that this warning is in master.
Re: doc: add missing "id" attributes to extension packaging page
On Tue, 4 Apr 2023 21:52:31 +0200 Brar Piening wrote: > On 04.04.2023 at 16:54, Peter Eisentraut wrote: > > The XSLT implementation looks sound to me. It would be a touch > > better if it had some comments about which parts of the templates > > were copied from upstream stylesheets and which were changed. I like this idea. A lot. (Including which stylesheets were copied from.) > > However, I wonder if this is the right way to approach this. I > > don't think we should put these link markers directly into the > > HTML. It feels like this is the wrong layer. For example, if you > > have CSS turned off, then all these # marks show up by default. > > I'd consider this a feature rather than a problem but this is > certainly debatable. I too would consider this a feature. If you don't style your html presentation, you see everything. The "#" links to content are, well, content. > > It seems to me that the correct way to do this is to hook in some > > JavaScript that does this transformation directly on the DOM. Then > > we don't need to carry this presentation detail in the HTML. I would argue the opposite. The HTML/CSS is delivered to the browser which is then free to present the content to the user in the form preferred by the user. This puts control of presentation in the hands of the end-user, where, IMO, it belongs. Using JavaScript to manipulate the DOM is all well and good when using AJAX to interact with the server to produce dynamic content. But otherwise manipulating the DOM with JavaScript seems overly heavy-handed, even though popular. It seems like JavaScript is used more because CSS is difficult and an "extra technology" when instead JavaScript can "do everything". So CSS is put aside. I may be biased, not being a JavaScript fan. (I tend to be one of those cranky individuals who keep JavaScript turned off.) I'd rather not have code executing when such overhead/complication can be avoided. (Insert here exciting argument about "what is code and what is data".) Glancing at the documentation source, I don't see JavaScript used at all. Introducing it would be adding something else to the mix. Not that this would be bad if it provides value. In the end, I don't _really_ care. And don't do web development all day either so my fundamentals could be just wrong. But this is my take. > > Moreover, it would avoid tight coupling between the website and the > > documentation sources. I don't really understand this statement. Are you saying that the documentation's source CSS needn't/shouldn't be the CSS used on the website? That seems counter-intuitive. But then I don't understand why the default CSS used when developing the documentation is not the CSS used on the website. I can imagine administrative arguments around server maintenance for wanting to keep the website decoupled from the PG source code. (I think.) But I can't speak to any of that. Regards, Karl Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein
Re: [BUG] pg_stat_statements and extended query protocol
> I was looking back at this thread, and the suggestion to use one field > in EState sounds fine to me. Sami, would you like to send a new > version of the patch (simplified version based on v1)? Here is v4. The "calls" tracking is removed from Estate. Unlike v1 however, I added a check for the operation type. Inside ExecutorRun, es_total_processed is incremented when the operation is a SELECT. This check is done for es_processed as well inside executorRun -> ExecutePlan. For non select operations, es_total_processed is set to es_processed in executorfinish. This is because the modify plan nodes set es_processed outside of execMain.c Regards, Sami Imseih Amazon Web Services (AWS) v4-0001-Fix-row-tracking-in-pg_stat_statements.patch Description: v4-0001-Fix-row-tracking-in-pg_stat_statements.patch
Re: Parallel Full Hash Join
On Wed, Apr 5, 2023 at 7:37 AM Tom Lane wrote: > The comment is no longer in sync with the code: this if-test used to > reject JOIN_FULL and JOIN_RIGHT, and no longer does so, but the comment > still claims it should. Shouldn't we drop the sentence beginning > "Similarly"? (I see that there's now one sub-section that still rejects > such cases, but it no longer seems correct to claim that they're rejected > overall.) Yeah, thanks. Done.
Re: Check whether binaries can be released for s390x
On Wed, Apr 5, 2023 at 3:03 AM Robert Haas wrote: > On Tue, Apr 4, 2023 at 9:30 AM Namrata Bhave wrote: > > Thank you for getting back. > > > > The request is mainly for the developer community to build and publish > > s390x binaries, apologies if I wasn't clear earlier. > > We can provide s390x VMs to build and test postgresql binaries if you feel > > infra is a blocker. > > > > Please let me know if any more information is needed. > > As Jonathon said, this discussion is not pertinent to this mailing > list. Please use the correct mailing list. > > I'm not sure that anyone is going to want to undertake this effort for > free even if you're willing to provide the hardware for free. But your > chances will be a lot better if you ask on the correct mailing list. Isn't the right place for this the APT packaging team[1]'s list at pgsql-pkg-deb...@postgresql.org? Not something I'm involved in myself, but AFAIK it's the same team working on Debian etc packages and they do get built for s390x already[2] so I guess this is about doing the same for the PGDG repos, so this is a packaging topic, not a web topic, and this list wasn't a bad place to start... [1] https://wiki.postgresql.org/wiki/Apt [2] https://packages.debian.org/search?arch=s390x&keywords=postgresql-15
Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
On Tue, Apr 04, 2023 at 09:04:34PM +0900, Michael Paquier wrote: > This addition looks OK for me. Thanks for the patch! Okay, finally done. One part that was still not complete to me in light of the information ddfc2d9 has removed is that the number of physical reads could be lower than the reported number depending on what the kernel cache holds. So I've added this sentence, while on it. -- Michael signature.asc Description: PGP signature
Re: monitoring usage count distribution
On Tue, Apr 4, 2023 at 2:40 PM Tom Lane wrote: > > Robert Haas writes: > > On Mon, Jan 30, 2023 at 6:30 PM Nathan Bossart > > wrote: > >> My colleague Jeremy Schneider (CC'd) was recently looking into usage count > >> distributions for various workloads, and he mentioned that it would be nice > >> to have an easy way to do $SUBJECT. > > > I'm skeptical that pg_buffercache_summary() is a good idea at all, but > > having it display the average usage count seems like a particularly > > poor idea. That information is almost meaningless. Replacing that with > > a six-element integer array would be a clear improvement and, IMHO, > > better than adding yet another function to the extension. > > I had not realized that pg_buffercache_summary() is new in v16, > but since it is, we still have time to rethink its definition. > +1 for de-aggregating --- I agree that the overall average is > unlikely to have much value. So, I have used pg_buffercache_summary() to give me a high-level idea of the usage count when I am benchmarking a particular workload -- and I would have found it harder to look at 6 rows instead of 1. That being said, having six rows is more versatile as you could aggregate it yourself easily. - Melanie
Re: psql \watch 2nd argument: iteration count
On Fri, Mar 24, 2023 at 10:32 PM Andrey Borodin wrote: > On Thu, Mar 23, 2023 at 10:15 PM Yugo NAGATA wrote: > > > > Here is my review on the v9 patch. > > > > + /* we do not prevent numerous names iterations like > i=1 i=1 i=1 */ > > + have_sleep = true; > > > > Why this is allowed here? I am not sure there is any reason to allow to > specify > > multiple "interval" options. (I would apologize it if I missed past > discussion.) > I do not know, it just seems normal to me. I've fixed this. > > > postgres=# select 1 \watch interval=3 4 > > \watch: incorrect interval value '4' > > > > I think it is ok, but this error message seems not user-friendly because, > > in the above example, interval values itself is correct, but it seems > just > > a syntax error. I wonder it is better to use "watch interval must be > specified > > only once" or such here, as the past patch. > Done. > > > > > + > > +If number of iterations is specified - query will be executed > only > > +given number of times. > > + > > > > Is it common to use "-" here? I think using comma like > > "If number of iterations is specified, " > > is natural. > Done. > > Thank for the review! > > > Best regards, Andrey Borodin. > Okay, I tested this. It handles bad param names, values correctly. Nice Feature, especially if you leave a 1hr task running and you need to step away... Built/Reviewed the Docs. They are correct. Reviewed \? command. It has the parameters updated, shown as optional Marked as Ready for Committer.
Re: fill_seq_fork_with_data() initializes buffer without lock
Hi,
On 2023-04-04 11:55:01 -0700, Andres Freund wrote:
> Look at:
>
> static void
> fill_seq_fork_with_data(Relation rel, HeapTuple tuple, ForkNumber forkNum)
> {
> Buffer buf;
> Pagepage;
> sequence_magic *sm;
> OffsetNumber offnum;
>
> /* Initialize first page of relation with special magic number */
>
> buf = ReadBufferExtended(rel, forkNum, P_NEW, RBM_NORMAL, NULL);
> Assert(BufferGetBlockNumber(buf) == 0);
>
> page = BufferGetPage(buf);
>
> PageInit(page, BufferGetPageSize(buf), sizeof(sequence_magic));
> sm = (sequence_magic *) PageGetSpecialPointer(page);
> sm->magic = SEQ_MAGIC;
>
> /* Now insert sequence tuple */
>
> LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
>
>
> Clearly we are modifying the page (via PageInit()), without holding a buffer
> lock, which is only acquired subsequently.
>
> It's clearly unlikely to cause bad consequences - the sequence doesn't yet
> really exist, and we haven't seen any reports of a problem - but it doesn't
> seem quite impossible that it would cause problems.
>
> As far as I can tell, this goes back to the initial addition of the sequence
> code, in e8647c45d66a - I'm too lazy to figure out whether it possibly wasn't
> a problem in 1997 for some reason.
Robert suggested to add an assertion to PageInit() to defend against such
omissions. I quickly hacked one together. The assertion immediately found the
issue here, but no other currently existing ones.
I'm planning to push a fix for this to HEAD. Given that the risk seems low and
the issue is so longstanding, it doesn't seem quite worth backpatching?
FWIW, the assertion I used is:
if (page >= BufferBlocks && page <= BufferBlocks + BLCKSZ * NBuffers)
{
Buffer buffer = (page - BufferBlocks) / BLCKSZ + 1;
BufferDesc *buf = GetBufferDescriptor(buffer - 1);
Assert(LWLockHeldByMeInMode(BufferDescriptorGetContentLock(buf),
LW_EXCLUSIVE));
}
If there's interest in having such an assertion permenantly, it clearly can't
live in bufpage.c.
I have a bit of a hard time coming up with a good name. Any suggestions?
Greetings,
Andres Freund
Re: monitoring usage count distribution
Hi, On 2023-04-04 14:14:36 -0400, Greg Stark wrote: > Tom expressed skepticism that there's wide interest here. It seems as > much from the lack of response. But perhaps that's just because people > don't understand what the importance of this info is -- I certainly > don't :) pg_buffercache has exposed the raw data for a long time. The problem is that it's way too slow to look at that way. Greetings, Andres Freund
Re: monitoring usage count distribution
Hi, On 2023-04-04 14:31:36 -0400, Robert Haas wrote: > On Mon, Jan 30, 2023 at 6:30 PM Nathan Bossart > wrote: > > My colleague Jeremy Schneider (CC'd) was recently looking into usage count > > distributions for various workloads, and he mentioned that it would be nice > > to have an easy way to do $SUBJECT. I've attached a patch that adds a > > pg_buffercache_usage_counts() function. This function returns a row per > > possible usage count with some basic information about the corresponding > > buffers. > > > > postgres=# SELECT * FROM pg_buffercache_usage_counts(); > > usage_count | buffers | dirty | pinned > > -+-+---+ > >0 | 0 | 0 | 0 > >1 |1436 | 671 | 0 > >2 | 102 |88 | 0 > >3 | 23 |21 | 0 > >4 | 9 | 7 | 0 > >5 | 164 | 106 | 0 > > (6 rows) > > > > This new function provides essentially the same information as > > pg_buffercache_summary(), but pg_buffercache_summary() only shows the > > average usage count for the buffers in use. If there is interest in this > > idea, another approach to consider could be to alter > > pg_buffercache_summary() instead. > > I'm skeptical that pg_buffercache_summary() is a good idea at all Why? It's about two orders of magnitude faster than querying the equivalent data by aggregating in SQL. And knowing how many free and dirty buffers are over time is something quite useful to monitor / correlate with performance issues. > but having it display the average usage count seems like a particularly poor > idea. That information is almost meaningless. I agree there are more meaningful ways to represent the data, but I don't agree that it's almost meaningless. It can give you a rough estimate of whether data in s_b is referenced or not. > Replacing that with a six-element integer array would be a clear improvement > and, IMHO, better than adding yet another function to the extension. I'd have no issue with that. Greetings, Andres Freund
CREATE SUBSCRIPTION -- add missing tab-completes
There are some recent comment that added new options for CREATE SUBSCRIPTION "Add new predefined role pg_create_subscription." [1] This added a new "password_required" option. "Add a run_as_owner option to subscriptions." [2] This added a "run_as_owner" option. ~~ AFAICT the associated tab-complete code was accidentally omitted. PSA patches to add those tab completions. -- [1] https://github.com/postgres/postgres/commit/c3afe8cf5a1e465bd71e48e4bc717f5bfdc7a7d6 [2] https://github.com/postgres/postgres/commit/482675987bcdffb390ae735cfd5f34b485ae97c6 Kind Regards, Peter Smith. Fujitsu Australia v1-0002-Add-tab-completion-for-SUBSCRIPTION-option-run_as.patch Description: Binary data v1-0001-Add-tab-completion-for-SUBSCRIPTION-option-passwo.patch Description: Binary data
Comment typo in recent push
There seems to be a comment typo in the recent commit "Perform logical replication actions as the table owner" [1]. /* * Switch back to the original user ID. * * If we created a new GUC nest level, also role back any changes that were * made within it. */ /role back/rollback/ ~~ PSA a tiny patch to fix that. -- [1] https://github.com/postgres/postgres/commit/1e10d49b65d6c26c61fee07999e4cd59eab2b765 Kind Regards, Peter Smith. Fujitsu Australia v1-0001-Fix-comment-typo.patch Description: Binary data
Re: refactoring relation extension and BufferAlloc(), faster COPY
Hi,
On 2023-03-30 12:28:57 +0700, John Naylor wrote:
> On Thu, Mar 30, 2023 at 10:02 AM Andres Freund wrote:
> > Attached is v6. Changes:
>
> 0006:
>
> +ereport(ERROR,
> +errcode_for_file_access(),
> +errmsg("could not extend file \"%s\" with posix_fallocate():
> %m",
> + FilePathName(v->mdfd_vfd)),
> +errhint("Check free disk space."));
>
> Portability nit: mdzeroextend() doesn't know whether posix_fallocate() was
> used in FileFallocate().
Fair point. I would however like to see a different error message for the two
ways of extending, at least initially. What about referencing FileFallocate()?
Greetings,
Andres Freund
Re: Minimal logical decoding on standbys
On Tue, 2023-04-04 at 14:55 -0400, Robert Haas wrote: > Thanks for your continued work on $SUBJECT. I just took a look at > 0004, and I think that at the very least the commit message needs > work. Nobody who is not a hacker is going to understand what problem > this is fixing, because it makes reference to the names of functions > and structure members rather than user-visible behavior. In fact, I'm > not really sure that I understand the problem myself. It seems like > the problem is that on a standby, WAL senders will get woken up too > early, before we have any WAL to send. Logical walsenders on the standby, specifically, which didn't exist before this patch series. > That's presumably OK, in the > sense that they'll go back to sleep and eventually wake up again, but > it means they might end up chronically behind sending out WAL to > cascading standbys. Without 0004, cascading logical walsenders would have worse wakeup behavior than logical walsenders on the primary. Assuming the fix is small in scope and otherwise acceptable, I think it belongs as a part of this overall series. > If that's right, I think it should be spelled out > more clearly in the commit message, and maybe also in the code > comments. Perhaps a commit message like: "For cascading replication, wake up physical walsenders separately from logical walsenders. Physical walsenders can't send data until it's been flushed; logical walsenders can't decode and send data until it's been applied. On the standby, the WAL is flushed first, which will only wake up physical walsenders; and then applied, which will only wake up logical walsenders. Previously, all walsenders were awakened when the WAL was flushed. That was fine for logical walsenders on the primary; but on the standby the flushed WAL would not have been applied yet, so logical walsenders were awakened too early." (I'm not sure if I quite got the verb tenses right.) For comments, I agree that WalSndWakeup() clearly needs a comment update. The call site in ApplyWalRecord() could also use a comment. You could add a comment at every call site, but I don't think that's necessary if there's a good comment over WalSndWakeup(). Regards, Jeff Davis
Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)
On Tue, Apr 4, 2023 at 4:35 PM Pavel Luzanov wrote:
>
> On 03.04.2023 23:50, Melanie Plageman wrote:
> > Attached is a tiny patch to add standalone backend type to
> > pg_stat_activity documentation (referenced by pg_stat_io).
> >
> > I mentioned both the bootstrap process and single user mode process in
> > the docs, though I can't imagine that the bootstrap process is relevant
> > for pg_stat_activity.
>
> After a little thought... I'm not sure about the term 'bootstrap
> process'. I can't find this term in the documentation.
There are various mentions of "bootstrap" peppered throughout the docs
but no concise summary of what it is. For example, initdb docs mention
the "bootstrap backend" [1].
Interestingly, 910cab820d0 added "Bootstrap superuser" in November. This
doesn't really cover what bootstrapping is itself, but I wonder if that
is useful? If so, you could propose a glossary entry for it?
(preferably in a new thread)
> Do I understand correctly that this is a postmaster? If so, then the
> postmaster process is not shown in pg_stat_activity.
No, bootstrap process is for initializing the template database. You
will not be able to see pg_stat_activity when it is running.
> Perhaps it may be worth adding a description of the standalone backend
> to pg_stat_io, not to pg_stat_activity.
> Something like: backend_type is all types from pg_stat_activity plus
> 'standalone backend',
> which is used for the postmaster process and in a single user mode.
You can query pg_stat_activity from single user mode, so it is relevant
to pg_stat_activity also. I take your point that bootstrap mode isn't
relevant for pg_stat_activity, but I am hesitant to add that distinction
to the pg_stat_io docs since the reason you won't see it in
pg_stat_activity is because it is ephemeral and before a user can access
the database and not because stats are not tracked for it.
Can you think of a way to convey this?
> > I also noticed that the pg_stat_activity docs call background workers
> > "parallel workers" (though it also mentions that extensions could have
> > other background workers registered), but this seems a bit weird because
> > pg_stat_activity uses GetBackendTypeDesc() and this prints "background
> > worker" for type B_BG_WORKER. Background workers doing parallelism tasks
> > is what users will most often see in pg_stat_activity, but I feel like
> > it is confusing to have it documented as something different than what
> > would appear in the view. Unless I am misunderstanding something...
>
> 'parallel worker' appears in the pg_stat_activity for parallel queries.
> I think it's right here.
Ah, I didn't read the code closely enough in pg_stat_get_activity()
Even though there is no BackendType which GetBackendTypeDesc() returns
called "parallel worker", we to out of our way to be specific using
GetBackgroundWorkerTypeByPid()
/* Add backend type */
if (beentry->st_backendType == B_BG_WORKER)
{
const char *bgw_type;
bgw_type = GetBackgroundWorkerTypeByPid(beentry->st_procpid);
if (bgw_type)
values[17] = CStringGetTextDatum(bgw_type);
else
nulls[17] = true;
}
else
values[17] =
CStringGetTextDatum(GetBackendTypeDesc(beentry->st_backendType));
- Melanie
[1] https://www.postgresql.org/docs/current/app-initdb.html
Re: Minimal logical decoding on standbys
Hi, On 2023-04-04 17:33:25 -0700, Jeff Davis wrote: > On Tue, 2023-04-04 at 14:55 -0400, Robert Haas wrote: > > That's presumably OK, in the > > sense that they'll go back to sleep and eventually wake up again, but > > it means they might end up chronically behind sending out WAL to > > cascading standbys. > > Without 0004, cascading logical walsenders would have worse wakeup > behavior than logical walsenders on the primary. Assuming the fix is > small in scope and otherwise acceptable, I think it belongs as a part > of this overall series. FWIW, personally, I wouldn't feel ok with committing 0003 without 0004. And IMO they ought to be committed the other way round. The stalls you *can* get, depending on the speed of WAL apply and OS scheduling, can be long. This is actually why a predecessor version of the feature had a bunch of sleeps and retries in the tests, just to avoid those stalls. Obviously that's not a good path... Greetings, Andres Freund
Re: [PATCH] Add function to_oct
On Thu, Feb 23, 2023 at 6:32 AM Peter Eisentraut < peter.eisentr...@enterprisedb.com> wrote: > On 20.12.22 23:08, Eric Radman wrote: > > This patch is a new function based on the implementation of to_hex(int). > > > > Since support for octal integer literals was added, to_oct(int) allows > > octal values to be easily stored and returned in query results. > > > >to_oct(0o755) = '755' > > > > This is probably most useful for storing file system permissions. > > Note this subsequent discussion about the to_hex function: > > https://www.postgresql.org/message-id/flat/CAEZATCVbkL1ynqpsKiTDpch34%3DSCr5nnau%3DnfNmiy2nM3SJHtw%40mail.gmail.com > > Also, I think there is no "to binary" function, so perhaps if we're > going down this road one way or the other, we should probably complete > the set. > > The code reads clearly. It works as expected (being an old PDP-11 guy!)... And the docs make sense and build as well. Nothing larger than an int gets in. I was "missing" the bigint version, but read through ALL of the messages to see (and agree) That that's okay. Marked Ready for Committer. Thanks, Kirk
