Re: PSQL error: total cell count of XXX exceeded
On Mon, 11 Sept 2023 at 08:51, Hongxu Ma wrote: > > I created a patch to fix it. > Really appreciate to anyone can help to review it. > Thanks. I think "product" as a variable name isn't very descriptive. Let's call it total_cells (or something similar instead). Other than that I think it's a good change. content->cellsadded is also a long, So I agree that I don't think the limit of int cells was intended here.
Re: pg_logical_emit_message() misses a XLogFlush()
On Mon, Sep 11, 2023 at 02:42:16PM +0900, bt23nguyent wrote:
> A minor issue with the description here is that while the description for
> the new flush argument in pg_logical_emit_message() with bytea type is
> clearly declared, there is no description of flush argument in the former
> pg_logical_emit_message() with text type at all.
Indeed, I forgot to update the first function signature. Fixed in the
attached.
> On a side note, could you also include a bit more information that "flush is
> set to false by default" in the document as well? It could be helpful for
> the users.
With the function signature saying that, that did not seem stricly
necessary to me, but no objections to add a few words about that.
I'll need a bit more input from Fujii-san before doing anything about
his comments, still it looks like a doc issue to me that may need a
backpatch to clarify how the non-transactional case behaves.
Attaching a v4 with the two doc changes, fow now.
--
Michael
From fea24a7f6f5451388945307bdd2ebefd6e5ecd15 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 11 Sep 2023 16:14:02 +0900
Subject: [PATCH v4] Add flush argument to pg_logical_emit_message()
The default is false, to not flush messages. If set to true, the
message is flushed before returning back to the caller.
---
src/include/catalog/pg_proc.dat | 4 ++--
src/include/replication/message.h | 3 ++-
src/backend/catalog/system_functions.sql | 20 +++
.../replication/logical/logicalfuncs.c| 3 ++-
src/backend/replication/logical/message.c | 13 ++--
doc/src/sgml/func.sgml| 9 +++--
contrib/test_decoding/expected/messages.out | 5 +++--
contrib/test_decoding/sql/messages.sql| 5 +++--
8 files changed, 50 insertions(+), 12 deletions(-)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 9805bc6118..3e52e0d4ac 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -11158,11 +11158,11 @@
prosrc => 'pg_replication_slot_advance' },
{ oid => '3577', descr => 'emit a textual logical decoding message',
proname => 'pg_logical_emit_message', provolatile => 'v', proparallel => 'u',
- prorettype => 'pg_lsn', proargtypes => 'bool text text',
+ prorettype => 'pg_lsn', proargtypes => 'bool text text bool',
prosrc => 'pg_logical_emit_message_text' },
{ oid => '3578', descr => 'emit a binary logical decoding message',
proname => 'pg_logical_emit_message', provolatile => 'v', proparallel => 'u',
- prorettype => 'pg_lsn', proargtypes => 'bool text bytea',
+ prorettype => 'pg_lsn', proargtypes => 'bool text bytea bool',
prosrc => 'pg_logical_emit_message_bytea' },
# event triggers
diff --git a/src/include/replication/message.h b/src/include/replication/message.h
index 6ce7f2038b..0f168d572c 100644
--- a/src/include/replication/message.h
+++ b/src/include/replication/message.h
@@ -30,7 +30,8 @@ typedef struct xl_logical_message
#define SizeOfLogicalMessage (offsetof(xl_logical_message, message))
extern XLogRecPtr LogLogicalMessage(const char *prefix, const char *message,
- size_t size, bool transactional);
+ size_t size, bool transactional,
+ bool flush);
/* RMGR API */
#define XLOG_LOGICAL_MESSAGE 0x00
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index 07c0d89c4f..714f5da941 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -446,6 +446,26 @@ LANGUAGE INTERNAL
VOLATILE ROWS 1000 COST 1000
AS 'pg_logical_slot_peek_binary_changes';
+CREATE OR REPLACE FUNCTION pg_catalog.pg_logical_emit_message(
+transactional boolean,
+prefix text,
+message text,
+flush boolean DEFAULT false)
+RETURNS pg_lsn
+LANGUAGE INTERNAL
+VOLATILE STRICT
+AS 'pg_logical_emit_message_text';
+
+CREATE OR REPLACE FUNCTION pg_catalog.pg_logical_emit_message(
+transactional boolean,
+prefix text,
+message bytea,
+flush boolean DEFAULT false)
+RETURNS pg_lsn
+LANGUAGE INTERNAL
+VOLATILE STRICT
+AS 'pg_logical_emit_message_bytea';
+
CREATE OR REPLACE FUNCTION pg_create_physical_replication_slot(
IN slot_name name, IN immediately_reserve boolean DEFAULT false,
IN temporary boolean DEFAULT false,
diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
index 197169d6b0..1067aca08f 100644
--- a/src/backend/replication/logical/logicalfuncs.c
+++ b/src/backend/replication/logical/logicalfuncs.c
@@ -362,10 +362,11 @@ pg_logical_emit_message_bytea(PG_FUNCTION_ARGS)
bool transactional = PG_GETARG_BOOL(0);
char *prefix = text_to_cstring(PG_GETARG_TEXT_PP(1));
bytea *data = PG_GETARG_BYTEA_PP(2);
+ bool flush = PG_GETARG_BOOL(3);
XLogRecPtr lsn;
lsn = LogLogicalMessage(prefix, VARDATA_ANY(data), VARSIZE_ANY_EXHDR(data),
- transactional);
+
Re: MergeJoin beats HashJoin in the case of multiple hash clauses
On Mon, Sep 11, 2023, at 11:51 AM, Andy Fan wrote: > Hi, > > On Thu, Jun 15, 2023 at 4:30 PM Andrey Lepikhov > wrote: >> Hi, all. >> >> Some of my clients use JOIN's with three - four clauses. Quite >> frequently, I see complaints on unreasonable switch of JOIN algorithm to >> Merge Join instead of Hash Join. Quick research have shown one weak >> place - estimation of an average bucket size in final_cost_hashjoin (see >> q2.sql in attachment) with very conservative strategy. >> Unlike estimation of groups, here we use smallest ndistinct value across >> all buckets instead of multiplying them (or trying to make multivariate >> analysis). >> It works fine for the case of one clause. But if we have many clauses, >> and if each has high value of ndistinct, we will overestimate average >> size of a bucket and, as a result, prefer to use Merge Join. As the >> example in attachment shows, it leads to worse plan than possible, >> sometimes drastically worse. >> I assume, this is done with fear of functional dependencies between hash >> clause components. But as for me, here we should go the same way, as >> estimation of groups. > > I can reproduce the visitation you want to improve and verify the patch > can do it expectedly. I think this is a right thing to do. > >> The attached patch shows a sketch of the solution. > > I understand that this is a sketch of the solution, but the below > changes still > make me confused. > > + if (innerbucketsize > virtualbuckets) > + innerbucketsize = 1.0 / virtualbuckets; > > innerbucketsize is a fraction of rows in all the rows, so it is between > 0.0 and 1.0. > and virtualbuckets is the number of buckets in total (when considered > the mutli > batchs), how is it possible for 'innerbucketsize > virtualbuckets' ? > Am > I missing something? You are right here. I've made a mistake here. Changed diff is in attachment. -- Regards, Andrei Lepikhov fix_bucketsize_v2.diff Description: Binary data
Re: pg_rewind with cascade standby doesn't work well
> Consider a scenario like this, > > Server A: primary > Server B :replica of A > Server C :replica of B > > and somehow A down ,so B gets promoted. > Server A: down > Server B :new primary > Server C :replica of B > > In this case, pg_rewind can be used to reconstruct the cascade; the source is C and the target is A. > However, we get error as belows by running pg_rewind. > > ``` > pg_rewind: fetched file "global/pg_control", length 8192 > pg_rewind: source and target cluster are on the same timeline > pg_rewind: no rewind required > ``` To fix the above mentioned behavior of pg_rewind, I suggest to change the cascade standby's (i.e. server C's) minRecoveryPointTLI when it receives the new timeline information from the new primary (i.e. server B). When server B is promoted, it creates an end-of-recovery record by calling CreateEndOfRecoveryRecord(). (in xlog.c) And also updates B's minRecoveryPoint and minRecoveryPointTLI. ``` /* * Update the control file so that crash recovery can follow the timeline * changes to this point. */ LWLockAcquire(ControlFileLock, LW_EXCLUSIVE); ControlFile->minRecoveryPoint = recptr; ControlFile->minRecoveryPointTLI = xlrec.ThisTimeLineID; UpdateControlFile(); LWLockRelease(ControlFileLock); ``` Since C is a replica of B, the end-of-recovery record is replicated from B to C, so the record is replayed in C by xlog_redo(). The attached patch updates minRecoveryPoint and minRecoveryPointTLI at this point by mimicking CreateEndOfRecoveryRecord(). With this patch, you can run pg_rewind with cascade standby immediately. (without waiting for checkpointing) Thoughts? Masaki Kuwamura v1-0001-pg_rewind-Fix-bug-using-cascade-standby-as-source.patch Description: Binary data
Re: persist logical slots to disk during shutdown checkpoint
On Mon, Sep 11, 2023 at 12:08 PM Michael Paquier wrote: > > On Fri, Sep 08, 2023 at 11:50:37AM +0530, Amit Kapila wrote: > > On Fri, Sep 8, 2023 at 10:08 AM Michael Paquier wrote: > >> > >> > >> +/* > >> + * This is used to track the last persisted confirmed_flush LSN value > >> to > >> + * detect any divergence in the in-memory and on-disk values for the > >> same. > >> + */ > >> > >> "This value tracks is the last LSN where this slot's data has been > >> flushed to disk. > >> > > > > This makes the comment vague as this sounds like we are saving a slot > > corresponding to some LSN which is not the case. If you prefer this > > tone then we can instead say: "This value tracks the last > > confirmed_flush LSN flushed which is used during a checkpoint shutdown > > to decide if a logical slot's data should be forcibly flushed or not." > > Okay, that looks like an improvement over the term "persisted". > Changed accordingly. > >> This is used during a checkpoint shutdown to decide > >> if a logical slot's data should be forcibly flushed or not." > >> > >> Hmm. WAL senders are shut down *after* the checkpointer and *after* > >> the shutdown checkpoint is finished (see PM_SHUTDOWN and > >> PM_SHUTDOWN_2) because we want the WAL senders to acknowledge the > >> checkpoint record before shutting down the primary. > >> > > > > As per my understanding, this is not true for logical walsenders. As > > per code, while HandleCheckpointerInterrupts(), we call ShutdownXLOG() > > which sends a signal to walsender to stop and waits for it to stop. > > And only after that, did it write a shutdown checkpoint WAL record. > > After getting the InitStopping signal, walsender sets got_STOPPING > > flag. Then *logical* walsender ensures that it sends all the pending > > WAL and exits. What you have quoted is probably true for physical > > walsenders. > > Hm, reminding me about this area.. This roots down to the handling of > WalSndCaughtUp in the send_data callback for logical or physical. > This is switched to true for logical WAL senders much earlier than > physical WAL senders, aka before the shutdown checkpoint begins in the > latter. What was itching me a bit is that the postmaster logic could > be made more solid. Logical and physical WAL senders are both marked > as BACKEND_TYPE_WALSND, but we don't actually check that the WAL > senders remaining at the end of PM_SHUTDOWN_2 are *not* connected to a > database. This would require a new BACKEND_TYPE_* perhaps, or perhaps > we're fine with the current state because we'll catch up problems in > the checkpointer if any WAL is generated while the shutdown checkpoint > is running anyway. Just something I got in mind, unrelated to this > patch. > I don't know if the difference is worth inventing a new BACKEND_TYPE_* but if you think so then we can probably discuss this in a new thread. I think we may want to improve some comments as a separate patch to make this evident. > > + * We can flush dirty replication slots at regular intervals by any > + * background process like bgwriter but checkpoint is a convenient location. > > I still don't quite see a need to mention the bgwriter at all here.. > That's just unrelated. > I don't disagree with it, so changed it in the attached patch. > The comment block in CheckPointReplicationSlots() from v10 uses > "persist", but you mean "flush", I guess.. > This point is not very clear to me. Can you please quote the exact comment if you think something needs to be changed? -- With Regards, Amit Kapila. v11-0001-Persist-logical-slots-to-disk-during-a-shutdown-.patch Description: Binary data
Re: Pgoutput not capturing the generated columns
Hi PG Hackers. We are interested in enhancing the functionality of the pgoutput plugin by adding support for generated columns. Could you please guide us on the necessary steps to achieve this? Additionally, do you have a platform for tracking such feature requests? Any insights or assistance you can provide on this matter would be greatly appreciated. Many thanks. Rajendra.
Re: Correct the documentation for work_mem
On Sat, 9 Sept 2023 at 14:25, Imseih (AWS), Sami wrote: > > > This looks mostly fine to me modulo "sort or hash". I do see many > > instances of "and/or" in the docs. Maybe that would work better. > > "sort or hash operations at the same time" is clear explanation IMO. Just for anyone else following along that haven't seen the patch. The full text in question is: +Note that a complex query might perform several sort or hash +operations at the same time, with each operation generally being It's certainly not a show-stopper. I do believe the patch makes some improvements. The reason I'd prefer to see either "and" or "and/or" in place of "or" is because the text is trying to imply that many of these operations can run at the same time. I'm struggling to understand why, given that there could be many sorts and many hashes going on at once that we'd claim it could only be one *or* the other. If we have 12 sorts and 4 hashes then that's not "several sort or hash operations", it's "several sort and hash operations". Of course, it could just be sorts or just hashes, so "and/or" works fine for that. David
Re: pg_upgrade and logical replication
On Thu, 27 Apr 2023 at 13:18, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Julien,
>
> Thank you for updating the patch! Followings are my comments.
>
> 01. documentation
>
> In this page steps to upgrade server with pg_upgrade is aligned. Should we
> write
> down about subscriber? IIUC, it is sufficient to just add to "Run pg_upgrade",
> like "Apart from streaming replication standby, subscriber node can be upgrade
> via pg_upgrade. At that time we strongly recommend to use
> --preserve-subscription-state".
Now this option has been removed and made default
> 02. AlterSubscription
>
> I agreed that oid must be preserved between nodes, but I'm still afraid that
> given oid is unconditionally trusted and added to pg_subscription_rel.
> I think we can check the existenec of the relation via SearchSysCache1(RELOID,
> ObjectIdGetDatum(relid)). Of cource the check is optional, so it should be
> executed only when USE_ASSERT_CHECKING is on. Thought?
Modified
> 03. main
>
> Currently --preserve-subscription-state and --no-subscriptions can be used
> together, but the situation is quite unnatural. Shouldn't we exclude them?
This option is removed now, so this scenario will not happen
> 04. getSubscriptionTables
>
>
> ```
> + SubRelInfo *rels = NULL;
> ```
>
> The variable is used only inside the loop, so the definition should be also
> moved.
This logic is changed slightly, so it needs to be kept outside
> 05. getSubscriptionTables
>
> ```
> + nrels = atooid(PQgetvalue(res, i, i_nrels));
> ```
>
> atoi() should be used instead of atooid().
Modified
> 06. getSubscriptionTables
>
> ```
> + subinfo = findSubscriptionByOid(cur_srsubid);
> +
> + nrels = atooid(PQgetvalue(res, i, i_nrels));
> + rels = pg_malloc(nrels * sizeof(SubRelInfo));
> +
> + subinfo->subrels = rels;
> + subinfo->nrels = nrels;
> ```
>
> Maybe it never occurs, but findSubscriptionByOid() can return NULL. At that
> time
> accesses to their attributes will lead the Segfault. Some handling is needed.
This should not happen, added a fatal error in this case.
> 07. dumpSubscription
>
> Hmm, SubRelInfos are still dumped at the dumpSubscription(). I think this
> style
> breaks the manner of pg_dump. I think another dump function is needed. Please
> see dumpPublicationTable() and dumpPublicationNamespace(). If you have a
> reason
> to use the style, some comments to describe it is needed.
Modified
> 08. _SubRelInfo
>
> If you will address above comment, DumpableObject must be added as new
> attribute.
Modified
> 09. check_for_subscription_state
>
> ```
> + for (int i = 0; i < ntup; i++)
> + {
> + is_error = true;
> + pg_log(PG_WARNING,
> + "\nWARNING: subscription \"%s\"
> has an invalid remote_lsn",
> + PQgetvalue(res, 0, 0));
> + }
> ```
>
> The second argument should be i to report the name of subscription more than
> 2.
Modified
> 10. 003_subscription.pl
>
> ```
> $old_sub->wait_for_subscription_sync($publisher, 'sub');
>
> my $result = $old_sub->safe_psql('postgres',
> "SELECT COUNT(*) FROM pg_subscription_rel WHERE srsubstate != 'r'");
> is ($result, qq(0), "All tables in pg_subscription_rel should be in ready
> state");
> ```
>
> I think there is a possibility to cause a timing issue, because the SELECT may
> be executed before srsubstate is changed from 's' to 'r'. Maybe
> poll_query_until()
> can be used instead.
Modified
> 11. 003_subscription.pl
>
> ```
> command_ok(
> [
> 'pg_upgrade', '--no-sync','-d', $old_sub->data_dir,
> '-D', $new_sub->data_dir, '-b', $bindir,
> '-B', $bindir,'-s', $new_sub->host,
> '-p', $old_sub->port, '-P', $new_sub->port,
> $mode,
> '--preserve-subscription-state',
> '--check',
> ],
> 'run of pg_upgrade --check for old instance with correct sub rel');
> ```
>
> Missing check of pg_upgrade_output.d?
Modified
> And maybe you missed to run pgperltidy.
It has been run for the new patch.
The attached v7 patch has the changes for the same.
Regards,
Vignesh
From 4660c0914b8c3aef92461b84d7170ffc11bf5dd9 Mon Sep 17 00:00:00 2001
From: Vignesh C
Date: Thu, 7 Sep 2023 11:37:36 +0530
Subject: [PATCH v7] Preserve the full subscription's state during pg_upgrade
Previously, only the subscription metadata information was preserved. Without
the list of relations and their state it's impossible to re-enable the
subscriptions without missing some records as the list of relations can only be
refreshed after enabling the subscription (and therefore starting the apply
worker). Even
Re: pg_upgrade and logical replication
On Wed, 10 May 2023 at 13:29, Peter Smith wrote:
>
> Here are some review comments for the v5-0001 patch code.
>
> ==
> General
>
> 1. ALTER SUBSCRIPTION name ADD TABLE (relid = XYZ, state = 'x' [, lsn =
> 'X/Y'])
>
> I was a bit confused by this relation 'state' mentioned in multiple
> places. IIUC the pg_upgrade logic is going to reject anything with a
> non-READY (not 'r') state anyhow, so what is the point of having all
> the extra grammar/parse_subscription_options etc to handle setting the
> state when only possible value must be 'r'?
>
This command has been removed, this code has been removed
>
> 2. state V relstate
>
> I still feel code readbility suffers a bit by calling some fields/vars
> a generic 'state' instead of the more descriptive 'relstate'. Maybe
> it's just me.
>
> Previously commented same (see [1]#3, #4, #5)
Few of the code has been removed, I have modified wherever possible
> ==
> doc/src/sgml/ref/pgupgrade.sgml
>
> 3.
> +
> +Fully preserve the logical subscription state if any. That includes
> +the underlying replication origin with their remote LSN and the list
> of
> +relations in each subscription so that replication can be simply
> +resumed if the subscriptions are reactivated.
> +
>
> I think the "if any" part is not necessary. If you remove those words,
> then the rest of the sentence can be simplified.
>
> SUGGESTION
> Fully preserve the logical subscription state, which includes the
> underlying replication origin's remote LSN, and the list of relations
> in each subscription. This allows replication to simply resume when
> the subscriptions are reactivated.
>
This has been removed now.
>
> 4.
> +
> +If this option isn't used, it is up to the user to reactivate the
> +subscriptions in a suitable way; see the subscription part in +linkend="pg-dump-notes"/> for more information.
> +
>
> The link still renders strangely as previously reported (see [1]#2b).
>
This has been removed now
>
> 5.
> +
> +If this option is used and any of the subscription on the old cluster
> +has an unknown remote_lsn (0/0), or has any
> relation
> +in a state different from r (ready), the
> +pg_upgrade run will error.
> +
>
> 5a.
> /subscription/subscriptions/
Modified
> 5b
> "has any relation in a state different from r" --> "has any relation
> with state other than r"
Modified slightly
> ==
> src/backend/commands/subscriptioncmds.c
>
> 6.
> + if (strlen(state_str) != 1)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("invalid relation state: %s", state_str)));
>
> Is this relation state validation overly simplistic, by only checking
> for length 1? Shouldn't this just be asserting the relstate must be
> 'r'?
This code has been removed
> ==
> src/bin/pg_dump/pg_dump.c
>
> 7. getSubscriptionTables
>
> +/*
> + * getSubscriptionTables
> + * get information about the given subscription's relations
> + */
> +void
> +getSubscriptionTables(Archive *fout)
> +{
> + SubscriptionInfo *subinfo;
> + SubRelInfo *rels = NULL;
> + PQExpBuffer query;
> + PGresult *res;
> + int i_srsubid;
> + int i_srrelid;
> + int i_srsubstate;
> + int i_srsublsn;
> + int i_nrels;
> + int i,
> + cur_rel = 0,
> + ntups,
> + last_srsubid = InvalidOid;
>
> Why some above are single int declarations and some are compound int
> declarations? Why not make them all consistent?
Modified
> ~
>
> 8.
> + appendPQExpBuffer(query, "SELECT srsubid, srrelid, srsubstate, srsublsn,"
> + " count(*) OVER (PARTITION BY srsubid) AS nrels"
> + " FROM pg_subscription_rel"
> + " ORDER BY srsubid");
>
> Should this SQL be schema-qualified like pg_catalog.pg_subscription_rel?
Modified
> ~
>
> 9.
> + for (i = 0; i < ntups; i++)
> + {
> + int cur_srsubid = atooid(PQgetvalue(res, i, i_srsubid));
>
> Should 'cur_srsubid' be declared Oid to match the atooid?
Modified
> ~~~
>
> 10. getSubscriptions
>
> + if (PQgetisnull(res, i, i_suboriginremotelsn))
> + subinfo[i].suboriginremotelsn = NULL;
> + else
> + subinfo[i].suboriginremotelsn =
> + pg_strdup(PQgetvalue(res, i, i_suboriginremotelsn));
> +
> + /*
> + * For now assume there's no relation associated with the
> + * subscription. Later code might update this field and allocate
> + * subrels as needed.
> + */
> + subinfo[i].nrels = 0;
>
> The wording "For now assume there's no" kind of gives an ambiguous
> interpretation for this comment. IMO it sounds like this is the
> "current" logic but some future PG version may behave differently - I
> don't think that is the intended meaning at all.
>
> SUGGESTION.
> Here we just initialize nrels to say there are 0 relations associated
> with the subscription. If necessary, subsequent logic will update this
> field and allocate the subrels.
This part of logic has been removed now as it is no more required
> ~~~
>
> 11. dumpSubscription
>
> + for (i = 0; i < subinfo->
Re: generate syscache info automatically
On Thu, Aug 31, 2023 at 6:23 PM Peter Eisentraut
wrote:
> Attached is an updated patch set.
> There was some discussion about whether the catalog files are the right
> place to keep syscache information. Personally, I would find that more
> convenient. (Note that we also recently moved the definitions of
> indexes and toast tables from files with the whole list into the various
> catalog files.) But we can also keep it somewhere else. The important
> thing is that Catalog.pm can find it somewhere in a structured form.
I don't have anything further to say on how to fit it together, but I'll go
ahead share some other comments from a quick read of v3-0003:
+ # XXX hardcoded exceptions
+ # extension doesn't belong to extnamespace
+ $attnum_namespace = undef if $catname eq 'pg_extension';
+ # pg_database owner is spelled datdba
+ $attnum_owner = "Anum_pg_database_datdba" if $catname eq 'pg_database';
These exceptions seem like true exceptions...
+ # XXX?
+ $name_syscache = "SUBSCRIPTIONNAME" if $catname eq 'pg_subscription';
+ # XXX?
+ $is_nsp_name_unique = 1 if $catname eq 'pg_collation';
+ $is_nsp_name_unique = 1 if $catname eq 'pg_opclass';
+ $is_nsp_name_unique = 1 if $catname eq 'pg_opfamily';
+ $is_nsp_name_unique = 1 if $catname eq 'pg_subscription';
... but as the XXX conveys, these indicate a failure to do the right thing.
Trying to derive this info from the index declarations is currently
fragile. E.g. making one small change to this regex:
- if ($index->{index_decl} =~ /\(\w+name name_ops(,
\w+namespace oid_ops)?\)/)
+ if ($index->{index_decl} =~ /\w+name name_ops(,
\w+namespace oid_ops)?\)/)
...causes "is_nsp_name_unique" to flip from false to true, and/or sets
"name_catcache_id" where it wasn't before, for several entries. It's not
quite clear what the "rule" is intended to be, or whether it's robust going
forward.
That being the case, perhaps some effort should also be made to make it
easy to verify the output matches HEAD (or rather, v3-0001). This is now
difficult because of the C99 ".foo = bar" syntax, plus the alphabetical
ordering.
+foreach my $catname (@catnames)
+{
+ print $objectproperty_info_fh qq{#include "catalog/${catname}_d.h"\n};
+}
Assuming we have a better way to know which catalogs need
object properties, is there a todo to only #include those?
> To finish up the ObjectProperty generation, we also need to store some
> more data, namely the OBJECT_* mappings. We probably do not want to
> store those in the catalog headers; that looks like a layering violation
> to me.
Possibly, but it's also convenient and, one could argue, more
straightforward than storing syscache id and num_buckets in the index info.
One last thing: I did try to make the hash function use uint16 for the key
(to reduce loop iterations on nul bytes), and that didn't help, so we are
left with the ideas I mentioned earlier.
(not changing CF status, because nothing specific is really required to
change at the moment, some things up in the air)
--
John Naylor
EDB: http://www.enterprisedb.com
Re: Build the docs if there are changes in docs and don't run other tasks if the changes are only in docs
Hi, Thanks for the reply! On Fri, 8 Sept 2023 at 11:05, Daniel Gustafsson wrote: > > > On 7 Sep 2023, at 18:06, Nazir Bilal Yavuz wrote: > > > if the changes are only in the docs, don't run > > all tasks except building the docs task; this could help to save more > > CI times. > > A related idea for docs in order to save CI time: if the changes are only in > internal docs, ie README files, then don't run any tasks at all. Looking at > src/backend/parser/README the last two commits only touched that file, and > while such patches might not be all that common, spending precious CI > resources > on them seems silly if we can avoid it. > > It doesn't have to be included in this, just wanted to bring it up as it's > related. I liked the idea, I am planning to edit the 0002 patch. CI won't run any tasks if the changes are only in the README files. > Almost, but not entirely. There are a set of scripts which generate content > for the docs based on files in src/, like src/backend/catalog/sql_features.txt > and src/include/parser/kwlist.h. If those source files change, or their > scripts, it would be helpful to build docs. Thanks! Are these the only files that are not in the doc subfolders but effect docs? Regards, Nazir Bilal Yavuz Microsoft
Re: proposal: psql: show current user in prompt
On Fri, 8 Sept 2023 at 21:08, Pavel Stehule wrote: > ok changed - there is minor problem - almost all PQ functions are of int > type, but ProtocolVersion is uint Your current implementation requires using the PG_PROTOCOL macros to compare versions. But clients cannot currently use those macros since they are not exported from libpq-fe.h, only from pqcomm.h which is then imported by libpq-int.h. (psql/command.c imports pcomm.h directly, but I don't think we should expect clients to do that). We could ofcourse export these macros from libpq-fe.h too. But then we'd need to document those macros too. > Using different mapping to int can be problematic - can be messy if we cannot > use PG_PROTOCOL macro. I see no big problems returning an unsigned or long from the new function (even if existing functions all returned int). But I don't even think that is necessary. Returning the following as an int from PQprotocolVersionFull would work fine afaict: return PG_PROTOCOL_MAJOR(version) * 1000 + PG_PROTOCOL_MINOR(version) This would give us one thousand minor versions for each major version. This seems fine for all practical purposes. Since postgres only releases a version once every year, we'd need a protocol bump every year for one thousand years for that to ever cause any problems. So I'd prefer this approach over making the PG_PROTOCOL macros a public interface.
Re: pg_upgrade and logical replication
On Mon, 4 Sept 2023 at 13:26, Amit Kapila wrote:
>
> On Mon, Sep 4, 2023 at 11:51 AM Amit Kapila wrote:
> >
> > On Wed, Jul 19, 2023 at 12:47 PM Michael Paquier
> > wrote:
> > >
> > > On Wed, May 10, 2023 at 05:59:24PM +1000, Peter Smith wrote:
> > > > 1. ALTER SUBSCRIPTION name ADD TABLE (relid = XYZ, state = 'x' [, lsn =
> > > > 'X/Y'])
> > > >
> > > > I was a bit confused by this relation 'state' mentioned in multiple
> > > > places. IIUC the pg_upgrade logic is going to reject anything with a
> > > > non-READY (not 'r') state anyhow, so what is the point of having all
> > > > the extra grammar/parse_subscription_options etc to handle setting the
> > > > state when only possible value must be 'r'?
> > >
> > > We are just talking about the handling of an extra DefElem in an
> > > extensible grammar pattern, so adding the state field does not
> > > represent much maintenance work. I'm OK with the addition of this
> > > field in the data set dumped, FWIW, on the ground that it can be
> > > useful for debugging purposes when looking at --binary-upgrade dumps,
> > > and because we aim at copying catalog contents from one cluster to
> > > another.
> > >
> > > Anyway, I am not convinced that we have any need for a parse-able
> > > grammar at all, because anything that's presented on this thread is
> > > aimed at being used only for the internal purpose of an upgrade in a
> > > --binary-upgrade dump with a direct catalog copy in mind, and having a
> > > grammar would encourage abuses of it outside of this context. I think
> > > that we should aim for simpler than what's proposed by the patch,
> > > actually, with either a single SQL function à-la-binary_upgrade() that
> > > adds the contents of a relation. Or we can be crazier and just create
> > > INSERT queries for pg_subscription_rel to provide an exact copy of the
> > > catalog contents. A SQL function would be more consistent with other
> > > objects types that use similar tricks, see
> > > binary_upgrade_create_empty_extension() that does something similar
> > > for some pg_extension records. So, this function would require in
> > > input 4 arguments:
> > > - The subscription name or OID.
> > > - The relation OID.
> > > - Its LSN.
> > > - Its sync state.
> > >
> >
> > +1 for doing it via function (something like
> > binary_upgrade_create_sub_rel_state). We already have the internal
> > function AddSubscriptionRelState() that can do the core work.
> >
Modified
> One more related point:
> @@ -4814,9 +4923,31 @@ dumpSubscription(Archive *fout, const
> SubscriptionInfo *subinfo)
> if (strcmp(subinfo->subpasswordrequired, "t") != 0)
> appendPQExpBuffer(query, ", password_required = false");
>
> + if (dopt->binary_upgrade && dopt->preserve_subscriptions &&
> + subinfo->suboriginremotelsn)
> + {
> + appendPQExpBuffer(query, ", lsn = '%s'", subinfo->suboriginremotelsn);
> + }
>
> Even during Create Subscription, we can use an existing function
> (pg_replication_origin_advance()) or a set of functions to advance the
> origin instead of introducing a new option.
Added a function binary_upgrade_sub_replication_origin_advance which
will: a) check if the subscription exists, b) get the replication name
for subscription and c) advance the replication origin.
These are handled as part of v7 posted at [1].
[1] -
https://www.postgresql.org/message-id/CALDaNm1ZrbHaWpJwwNhDTJocRKWd3rEkgJazuDdZ9Z-WdvonFg%40mail.gmail.com
Regards,
Vignesh
Re: pg_upgrade and logical replication
On Wed, 10 May 2023 at 13:39, Peter Smith wrote: > > On Mon, Apr 24, 2023 at 4:19 PM Julien Rouhaud wrote: > > > > Hi, > > > > On Thu, Apr 13, 2023 at 03:26:56PM +1000, Peter Smith wrote: > > > > > > 1. > > > All the comments look alike, so it is hard to know what is going on. > > > If each of the main test parts could be highlighted then the test code > > > would be easier to read IMO. > > > > > > Something like below: > > > [...] > > > > I added a bit more comments about what's is being tested. I'm not sure > > that a > > big TEST CASE prefix is necessary, as it's not really multiple separated > > test > > cases and other stuff can be tested in between. Also AFAICT no other TAP > > test > > current needs this kind of banner, even if they're testing more complex > > scenario. > > Hmm, I think there are plenty of examples of subscription TAP tests > having some kind of highlighted comments as suggested, for better > readability. > > e.g. See src/test/subscription > t/014_binary.pl > t/015_stream.pl > t/016_stream_subxact.pl > t/018_stream_subxact_abort.pl > t/021_twophase.pl > t/022_twophase_cascade.pl > t/023_twophase_stream.pl > t/028_row_filter.pl > t/030_origin.pl > t/031_column_list.pl > t/032_subscribe_use_index.pl > > A simple to separate the main test parts is all > that is needed. Modified > > > > 4b. > > > All these messages like "Table t1 should still have 2 rows on the new > > > subscriber" don't seem very helpful. e.g. They are not saying anything > > > about WHAT this is testing or WHY it should still have 2 rows. > > > > I don't think that those messages are supposed to say what or why something > > is > > tested, just give a quick context / reference on the test in case it's > > broken. > > The comments are there to explain in more details what is tested and/or why. > > > > But, why can’t they do both? They can be a quick reference *and* at > the same time give some more meaning to the error log. Otherwise, > these messages might as well just say ‘ref1’, ‘ref2’, ‘ref3’... Modified These are handled as part of v7 posted at [1]. [1] - https://www.postgresql.org/message-id/CALDaNm1ZrbHaWpJwwNhDTJocRKWd3rEkgJazuDdZ9Z-WdvonFg%40mail.gmail.com Regards, Vignesh
Re: Possibility to disable `ALTER SYSTEM`
Hi, > I would like to propose a patch that allows administrators to disable `ALTER > SYSTEM` via either a runt-time option to pass to the Postgres server process > at startup (e.g. `--disable-alter-system=true`, false by default) or a new > GUC (or even both), without changing the current default method of the server. I'm actually going to put a strong +1 to Gabriele's proposal. It's an undeniable problem (I'm only seeing arguments regarding other ways the system would be insecure), and there might be real use cases for users outside kubernetes for having this feature and using it (meaning disabling the use of ALTER SYSTEM). In Patroni for example, the PostgreSQL service is controlled on all nodes by Patroni, and these kinds of changes could end up breaking the cluster if there was a failover. For this reason Patroni starts postgres with some GUC options as CMD arguments so that values in postgresql.conf or postgresql.auto.conf are ignored. The values in the DCS are the ones that matter. ``` postgres 1171221 0.0 1.9 903560 55168 ?S10:16 0:00 /usr/pgsql-15/bin/postgres -D /opt/postgres/data --config-file=/opt/postgres/data/postgresql.conf --listen_addresses=0.0.0.0 --port=5432 --cluster_name=patroni-tpa --wal_level=logical --hot_standby=on --max_connections=250 --max_wal_senders=6 --max_prepared_transactions=0 --max_locks_per_transaction=64 --track_commit_timestamp=off --max_replication_slots=6 --max_worker_processes=16 --wal_log_hints=on ``` (see more about how Patroni manages this here: https://patroni.readthedocs.io/en/latest/patroni_configuration.html#postgresql-parameters-controlled-by-patroni But let's face it, that's a hack, not something to be proud of, even if it does what is intended. And this is a product and we shouldn't be advertising hacks to overcome limitations. I find the opposition to this lacking good reasons, while I find the implementation to be useful in some cases. Kind regards, Martín -- Martín Marqués It’s not that I have something to hide, it’s that I have nothing I want you to see
Re: proposal: psql: show current user in prompt
@Tom and @Robert, since you originally suggested extending the protocol for this, I think some input from you on the protocol design would be quite helpful. BTW, this protocol extension is the main reason I personally care for this patch, because it would allow PgBouncer to ask for updates on certain GUCs so that it can preserve session level SET commands even in transaction pooling mode. Right now PgBouncer can only do this for a handful of GUCs, but there's quite a few others that are useful for PgBouncer to preserve by default: - search_path - statement_timeout - lock_timeout And users might have others that they want to preserve others too. On Fri, 8 Sept 2023 at 21:08, Pavel Stehule wrote: >> I think we'll need some bikeshedding on what the protocol message >> should look like exactly. I'm not entirely sure what is the most >> sensible here, so please treat everything I write next as >> suggestions/discussion: >> I see that you're piggy-backing on PQsendTypedCommand, which seems >> nice to avoid code duplication. It has one downside though: not every >> type, is valid for each command anymore. >> One way to avoid that would be to not introduce a new command, but >> only add a new type that is understood by Describe and Close, e.g. a >> 'G' (for GUC). Then PqMsg_Describe, G would become the equivalent of >> what'the current patch its PqMsg_ReportGUC, 't' and PqMsg_Close, G >> would be the same as PqMsg_ReportGUC, 'f'. > > > I am sorry, I don't understand this idea? To clarify what I meant: I meant instead of introducing a new top level message type (i.e. your newly introduced ReportGUC message), I suggested adding a new subtype that is understood by the Describe and Close messages. In addition to being able to use Describe and Close for statements and portals with the S and P subtypes respectively, like you currently can, we could add a new subtype G which would start and stop GUC reporting (with the Describe and Close message respectively). I think that would make the client code even simpler than it is now.
Re: Cleaning up array_in()
11.09.2023 08:26, jian he wrote:
hi.
Thanks for reviewing it.
DETAIL: Unexpected end of input.
In many cases, ending errors will happen, so I consolidate it.
SELECT '{{},}'::text[];
solved by tracking current token type and previous token type.
select '{\{}'::text[];
solved by update dstendptr.
attached.
Thank you!
I can confirm that all those anomalies are fixed now.
But new version brings a warning when compiled with gcc:
arrayfuncs.c:659:9: warning: variable 'prev_tok' is uninitialized when used
here [-Wuninitialized]
if (prev_tok == ATOK_DELIM || nest_level == 0)
^~~~
arrayfuncs.c:628:3: note: variable 'prev_tok' is declared here
ArrayToken prev_tok;
^
1 warning generated.
Also it looks like an updated comment needs fixing/improving:
/* No array dimensions, so first literal character should be oepn curl-braces
*/
(should be an opening brace?)
(I haven't look at the code closely.)
Best regards,
Alexander
Re: Speed up transaction completion faster after many relations are accessed in a transaction
Thank you for having a look at this. Apologies for not getting back to you sooner. On Wed, 5 Jul 2023 at 21:44, Heikki Linnakangas wrote: > > On 10/02/2023 04:51, David Rowley wrote: > > I've attached another set of patches. I do need to spend longer > > looking at this. I'm mainly attaching these as CI seems to be > > highlighting a problem that I'm unable to recreate locally and I > > wanted to see if the attached fixes it. > > I like this patch's approach. > > > index 296dc82d2ee..edb8b6026e5 100644 > > --- a/src/backend/commands/discard.c > > +++ b/src/backend/commands/discard.c > > @@ -71,7 +71,7 @@ DiscardAll(bool isTopLevel) > > Async_UnlistenAll(); > > - LockReleaseAll(USER_LOCKMETHOD, true); > > + LockReleaseSession(USER_LOCKMETHOD); > > ResetPlanCache(); > > This assumes that there are no transaction-level advisory locks. I think > that's OK. It took me a while to convince myself of that, though. I > think we need a high level comment somewhere that explains what > assumptions we make on which locks can be held in session mode and which > in transaction mode. Isn't it ok because DISCARD ALL cannot run inside a transaction block, so there should be no locks taken apart from possibly session-level locks? I've added a call to LockAssertNoneHeld(false) in there. > > @@ -3224,14 +3206,6 @@ PostPrepare_Locks(TransactionId xid) > > Assert(lock->nGranted <= lock->nRequested); > > Assert((proclock->holdMask & ~lock->grantMask) == > > 0); > > > > - /* Ignore it if nothing to release (must be a > > session lock) */ > > - if (proclock->releaseMask == 0) > > - continue; > > - > > - /* Else we should be releasing all locks */ > > - if (proclock->releaseMask != proclock->holdMask) > > - elog(PANIC, "we seem to have dropped a bit > > somewhere"); > > - > > /* > > * We cannot simply modify proclock->tag.myProc to > > reassign > > * ownership of the lock, because that's part of > > the hash key and > > This looks wrong. If you prepare a transaction that is holding any > session locks, we will now transfer them to the prepared transaction. > And its locallock entry will be out of sync. To fix, I think we could > keep around the hash table that CheckForSessionAndXactLocks() builds, > and use that here. Good catch. I've modified the patch to keep the hash table built in CheckForSessionAndXactLocks around for longer so that we can check for session locks. I've attached an updated patch mainly to get CI checking this. I suspect something is wrong as subscription/015_stream is timing out. I've not gotten to the bottom of that yet. David v7-0001-wip-resowner-lock-release-all.patch Description: Binary data
Re: proposal: psql: show current user in prompt
On Mon, 11 Sept 2023 at 13:59, Jelte Fennema wrote: > I think that would make the client code even simpler than it is now. To be clear, I'm not saying we should do this. There's benefits to using a dedicated new message type too (e.g. clearer errors if a proxy like pgbouncer does not understand it).
Re: Eliminate redundant tuple visibility check in vacuum
On Fri, Sep 8, 2023 at 11:06 AM Robert Haas wrote: > > On Thu, Sep 7, 2023 at 6:23 PM Melanie Plageman > wrote: > > I mostly wanted to remove the NULL checks because I found them > > distracting (so, a stylistic complaint). However, upon further > > reflection, I actually think it is better if heap_page_prune_opt() > > passes NULL. heap_page_prune() has no error callback mechanism that > > could use it, and passing a valid value implies otherwise. Also, as > > you said, off_loc will always be InvalidOffsetNumber when > > heap_page_prune() returns normally, so having heap_page_prune_opt() > > pass a dummy value might actually be more confusing for future > > programmers. > > I'll look at the new patches more next week, but I wanted to comment > on this point. I think this is kind of six of one, half a dozen of the > other. It's not that hard to spot a variable that's only used in a > function call and never initialized beforehand or used afterward, and > if someone really feels the need to hammer home the point, they could > always name it dummy or dummy_loc or whatever. So this point doesn't > really carry a lot of weight with me. I actually think that the > proposed change is probably better, but it seems like such a minor > improvement that I get slightly reluctant to make it, only because > churning the source code for arguable points sometimes annoys other > developers. > > But I also had the thought that maybe it wouldn't be such a terrible > idea if heap_page_prune_opt() actually used off_loc for some error > reporting goodness. I mean, if HOT pruning fails, and we don't get the > detail as to which tuple caused the failure, we can always run VACUUM > and it will give us that information, assuming of course that the same > failure happens again. But is there any reason why HOT pruning > shouldn't include that error detail? If it did, then off_loc would be > passed by all callers, at which point we surely would want to get rid > of the branches. This is a good idea. I will work on a separate patch set to add an error context callback for on-access HOT pruning. - Melanie
Re: Build the docs if there are changes in docs and don't run other tasks if the changes are only in docs
> On 11 Sep 2023, at 13:03, Nazir Bilal Yavuz wrote: >> Almost, but not entirely. There are a set of scripts which generate content >> for the docs based on files in src/, like >> src/backend/catalog/sql_features.txt >> and src/include/parser/kwlist.h. If those source files change, or their >> scripts, it would be helpful to build docs. > > Thanks! Are these the only files that are not in the doc subfolders > but effect docs? I believe so, the list of scripts and input files can be teased out of the doc/src/sgml/meson.build file. -- Daniel Gustafsson
Re: CHECK Constraint Deferrable
On Fri, Sep 8, 2023 at 1:23 PM Dilip Kumar wrote:
> 2.
> - if ((failed = ExecRelCheck(resultRelInfo, slot, estate)) != NULL)
> + if ((failed = ExecRelCheck(resultRelInfo, slot, estate,
> checkConstraint, &recheckConstraints)) != NULL && !recheckConstraints)
>
>
> Why recheckConstraints need to get as output from ExecRelCheck? I
> mean whether it will be rechecked or nor is already known by the
> caller and
>
Yes it will be known to the caller but ExecRelCheck will set this new
parameter only if any one of the constraint is defined as Deferrable (in
create table statement) and there is a potential constraint violation.
> Whether the constraint failed or passed is known based on the return
> value so why do you need to extra parameter here?
>
> Because if normal CHECK constraint(non Deferrable) is violated, no need to
proceed with the insertion and in that case recheckConstraints will hold
"false" but if Deferrable check constraint is violated, we need to
revalidate the constraint at commit time and recheckConstraints will hold
"true".
>
> 5.
> +typedef enum checkConstraintRecheck
> +{
> + CHECK_RECHECK_DISABLED, /* Recheck of CHECK constraint is disabled, so
> + * DEFERRED CHECK constraint will be
> + * considered as non-deferrable check
> + * constraint. */
> + CHECK_RECHECK_ENABLED, /* Recheck of CHECK constraint is enabled, so
> + * CHECK constraint will be validated but
> + * error will not be reported for deferred
> + * CHECK constraint. */
> + CHECK_RECHECK_EXISTING /* Recheck of existing violated CHECK
> + * constraint, indicates that this is a
> + * deferred recheck of a row that was reported
> + * as a potential violation of CHECK
> + * CONSTRAINT */
> +} checkConstraintRecheck;
>
> I do not like the naming convention here, especially the words
> RECHECK, DISABLE, and ENABLE. And also the name of the enum is a bit
> off. We can name it more like a unique constraint
> YES, PARTIAL, EXISTING
>
> I can think of alternative ENUM name as "EnforceDeferredCheck" and member
as “CHECK_DEFERRED_YES”, “CHECK_DEFRRED_NO” and “CHECK_DEFERRED_EXISTING”.
Thoughts?
--
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com
RE: [PoC] pg_upgrade: allow to upgrade publisher node
Dear Dilip,
Thank you for reviewing! PSA new version.
>
> 1.
> Note that slot restoration must be done after the final pg_resetwal command
> during the upgrade because pg_resetwal will remove WALs that are required by
> the slots. Due to this restriction, the timing of restoring replication slots
> is
> different from other objects.
>
> This comment in the commit message is confusing. I understand the
> reason but from this, it is not very clear that if resetwal removes
> the WAL we needed then why it is good to create after the resetwal. I
> think we should make it clear that creating new slot will set the
> restart lsn to current WAL location and after that resetwal can remove
> those WAL where slot restart lsn is pointing
Just to confirm - WAL records must not be removed in any time if it is referred
as restart_lsn. The reason why the slot creation is done after pg_restwal is
that
required WALs are not removed by the command. See [1].
Moreover, clarified more in the commit message.
> 2.
>
> +
> +
> +
> + All slots on the old cluster must be usable, i.e., there are no slots
> + whose
> +linkend="view-pg-replication-slots">pg_replication_slots.
> wal_status
> + is lost.
> +
> +
> +
> +
> +linkend="view-pg-replication-slots">pg_replication_slots.c
> onfirmed_flush_lsn
> + of all slots on the old cluster must be the same as the latest
> + checkpoint location.
> +
> +
> +
> +
> + The output plugins referenced by the slots on the old cluster must be
> + installed in the new PostgreSQL executable directory.
> +
> +
> +
> +
> + The new cluster must have
> +linkend="guc-max-replication-slots">max_replication_slots me>
> + configured to a value greater than or equal to the number of slots
> + present in the old cluster.
> +
> +
> +
> +
> + The new cluster must have
> +linkend="guc-wal-level">wal_level as
> + logical.
> +
> +
> +
>
> I think we should also add that the new slot should not have any
> permanent existing logical replication slot.
Hmm, I wondered it should be really needed. Tables are required not to be in the
new cluster too, but not documented. It might be a trivial thing. Anyway, added.
FYI - the restriction was not introduced by the patch. I reported independently
[2],
but no one has responded since now...
> 3.
> - with the primary.) Replication slots are not copied and must
> - be recreated.
> + with the primary.) Replication slots on the old standby are not
> copied.
> + Only logical slots on the primary are migrated to the new standby,
> + and other slots must be recreated.
>
> This paragraph should be rephrased. I mean first stating that
> "Replication slots on the old standby are not copied" and then saying
> Only logical slots are migrated doesn't seem like the best way. Maybe
> we can just say "Only logical slots on the primary are migrated to the
> new standby, and other slots must be recreated."
Per discussion on [3], I used another words. Thanks for suggesting.
> 4.
> + /*
> + * Raise an ERROR if the logical replication slot is invalidating. It
> + * would not happen because max_slot_wal_keep_size is set to -1 during
> + * the upgrade, but it stays safe.
> + */
> + if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade)
> + elog(ERROR, "Replication slots must not be invalidated during the
> upgrade.");
>
> Rephrase the first line as -> Raise an ERROR if the logical
> replication slot is invalidating during an upgrade.
Per discussion on [3], I used another words. Thanks for suggesting.
> 5.
> + /* Logical slots can be migrated since PG17. */
> + if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600)
> + return;
>
>
> For readability change this to if
> (GET_MAJOR_VERSION(old_cluster.major_version) < 1700), because in most
> of the checks related to this, we are using 1700 so better be
> consistent in this.
Per discussion on [3], I did not change here.
> 6.
> + if (nslots_on_new)
> + pg_fatal(ngettext("New cluster must not have logical replication
> slots but found %d slot.",
> + "New cluster must not have logical replication slots but found %d slots.",
> + nslots_on_new),
> + nslots_on_new);
> ...
> + if (PQntuples(res) != 1)
> + pg_fatal("could not determine wal_level.");
> +
> + wal_level = PQgetvalue(res, 0, 0);
> +
> + if (strcmp(wal_level, "logical") != 0)
> + pg_fatal("wal_level must be \"logical\", but is set to \"%s\"",
> + wal_level);
>
>
> I have noticed that the case of the first letter in the pg_fatal
> message is not consistent.
Actually there are some inconsistency even in the check.c file, so I devised
below rules. How do you think?
* Non-complete sentence starts with the lower case.
(e.g., "could not open", "could not determine")
* proper nouns are always noted with the lower
RE: [PoC] pg_upgrade: allow to upgrade publisher node
Dear Amit, Thank you for giving a suggestion! > > > > > 2. Why get_old_cluster_logical_slot_infos() need to use > > > pg_malloc_array whereas for similar stuff get_rel_infos() use > > > pg_malloc()? > > > > They did a same thing. I used pg_malloc_array() macro to keep the code > > within 80 columns. > > > > I think it is better to be consistent with the existing code in this > case. Also, see, if the usage in get_loadable_libraries() can also be > changed back to use pg_malloc(). Fixed as you said. The line becomes too long, so a variable was newly introduced. Best Regards, Hayato Kuroda FUJITSU LIMITED
Re: Possibility to disable `ALTER SYSTEM`
On Sat, Sep 9, 2023 at 5:14 PM Alvaro Herrera wrote: > > On 2023-Sep-08, Magnus Hagander wrote: > > > Now, it might be that you don't care at all about the *security* side > > of the feature, and only care about the convenience side. But in that > > case, the original suggestion from Tom of using an even trigger seems > > like a fine enough solution? > > ALTER SYSTEM, like all system-wide commands, does not trigger event > triggers. These are per-database only. > > https://www.postgresql.org/docs/16/event-trigger-matrix.html Hah, didn't think of that. And yes, that's a very good point. But one way to fix that would be to actually make event triggers for system wide commands, which would then be useful for other things as well... -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Re: Possibility to disable `ALTER SYSTEM`
On Mon, Sep 11, 2023 at 1:56 PM Martín Marqués wrote:
>
> Hi,
>
> > I would like to propose a patch that allows administrators to disable
> > `ALTER SYSTEM` via either a runt-time option to pass to the Postgres server
> > process at startup (e.g. `--disable-alter-system=true`, false by default)
> > or a new GUC (or even both), without changing the current default method of
> > the server.
>
> I'm actually going to put a strong +1 to Gabriele's proposal. It's an
> undeniable problem (I'm only seeing arguments regarding other ways the
> system would be insecure), and there might be real use cases for users
> outside kubernetes for having this feature and using it (meaning
> disabling the use of ALTER SYSTEM).
If enough people are in favor of it *given the known issues with it*,
I can drop my objection to a "meh, but I still don't think it's a good
idea".
But to do that, there would need to be a very in-your-face warning in
the documentation about it like "note that this only disables the
ALTER SYSTEM command. It does not prevent a superuser from changing
the configuration remotely using other means".
For example, in the very simplest, wth the POC patch out there now, I
can still run:
postgres=# CREATE TEMP TABLE x(t text);
CREATE TABLE
postgres=# INSERT INTO x VALUES ('work_mem=1TB');
INSERT 0 1
postgres=# COPY x TO '/home/mha/postgresql/inst/head/data/postgresql.auto.conf';
COPY 1
postgres=# SELECT pg_reload_conf();
pg_reload_conf
t
(1 row)
postgres=# show work_mem;
work_mem
--
1TB
(1 row)
Or anything similar to that.
Yes, this is marginally harder than saying ALTER SYSTEM SET
work_mem='1TB', but only very very marginally so. And from a security
perspective, there is zero difference.
But we do also allow "trust" authentication which is another major
footgun from a security perspective, against which we only defend with
warnings, so that in itself is not a reason not to do the same here.
> In Patroni for example, the PostgreSQL service is controlled on all
> nodes by Patroni, and these kinds of changes could end up breaking the
> cluster if there was a failover. For this reason Patroni starts
> postgres with some GUC options as CMD arguments so that values in
> postgresql.conf or postgresql.auto.conf are ignored. The values in the
> DCS are the ones that matter.
Right. And patroni would need to continue to do that even with this
patch, because it also needs to override if somebody puts something in
postgresql.conf, no? Removing the defence against that seems like a
bad idea...
> (see more about how Patroni manages this here:
> https://patroni.readthedocs.io/en/latest/patroni_configuration.html#postgresql-parameters-controlled-by-patroni
>
> But let's face it, that's a hack, not something to be proud of, even
> if it does what is intended. And this is a product and we shouldn't be
> advertising hacks to overcome limitations.
It's in a way a hack. But it's not the fault of ALTER SYSTEM, as you'd
also have to manage postgresql.conf. One slightly less hacky part
might be to have patroni generate a config file of it's own and
include it with the highest priority -- at that point it *would* be
come a hack around ALTER SYSTEM, because ALTER SYSTEM has a higher
priority than any manual user config file. But it is not today.
Another idea to solve the problem could be to instead introduce a
specific configuration file (either hardcoded or an
include_final_parameter= parameter, in which case patroni or the
k8s operator could set that parameter on the command line and that
parameter only) that is parsed *after* postgresql.auto.conf and
thereby would override the manual settings. This file would explicilty
be documented as intended for this type of tooling, and when you have
a tool - whether patroni or another declarative operator - it owns
this file and can overwrite it with whatever it wants. And this would
also retain the ability to use ALTER SYSTEM SET for *other*
parameters, if they want to.
That's just a very quick idea and there may definitely be holes in it,
but I'm not sure those holes are any worse than what's suggested here,
and I do thin kit's cleaner.
> I find the opposition to this lacking good reasons, while I find the
> implementation to be useful in some cases.
Stopping ALTER SYSTEM SET without actually preventing the superuser
from doing the same thing as they were doing before would to me be at
least as much of a hack as what patroni does today is.
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
How to add built-in func?
I only add below:
Datum fake_dinstance2(PG_FUNCTION_ARGS)
{
PG_RETURN_INT16(0);
}
in src/backend/utils/adt/int8.c, and the I run “make install”,
But I can’t find the fake_distance2 in src/backend/utils/fmgrtab.c which is
generated by src/backend/utils/Gen_fmgrtab.pl. What else do I need to add?
Re: Possibility to disable `ALTER SYSTEM`
Hi Magnus, On Mon, 11 Sept 2023 at 16:04, Magnus Hagander wrote: > But to do that, there would need to be a very in-your-face warning in > the documentation about it like "note that this only disables the > ALTER SYSTEM command. It does not prevent a superuser from changing > the configuration remotely using other means". > Although I did not include any docs in the PoC patch, that's exactly the plan. So +1 from me. > Yes, this is marginally harder than saying ALTER SYSTEM SET > work_mem='1TB', but only very very marginally so. And from a security > perspective, there is zero difference. > Agree, but the primary goal is not security. Indeed, security requires a more holistic approach (and in my initial thread I deliberately did not mention all the knobs that the operator provides, with stricter and stricter default values, as I thought it was not relevant from a Postgres' PoV). However, as I explained in the patch PoC thread, the change is intended primarily to warn legitimate administrators in a configuration managed controlled environment that ALTER SYSTEM has been disabled for that system. These are environments where network access for a superuser is prohibited, but still possible for local SREs to log in via the container in the pod for incident resolution - very often this happens in high stress conditions and I believe that this gate will help them remind that if they want to change the settings they need to do it through the Kubernetes resources. So primarily: usability. Another idea to solve the problem could be to instead introduce a > specific configuration file (either hardcoded or an > include_final_parameter= parameter, in which case patroni or the > k8s operator could set that parameter on the command line and that > parameter only) that is parsed *after* postgresql.auto.conf and > thereby would override the manual settings. This file would explicilty > be documented as intended for this type of tooling, and when you have > a tool - whether patroni or another declarative operator - it owns > this file and can overwrite it with whatever it wants. And this would > also retain the ability to use ALTER SYSTEM SET for *other* > parameters, if they want to. > But that is exactly the whole point of this request: disable the last operation altogether. This option will easily give any operator (or deployment in a configuration management scenario) the possibility to ship a system that, out-of-the box, facilitates this one direction update of the configuration, allowing the observed state to easily reconcile with the desired one. Without breaking any existing deployment. > Stopping ALTER SYSTEM SET without actually preventing the superuser > from doing the same thing as they were doing before would to me be at > least as much of a hack as what patroni does today is. > Agree, but as I said above, that'd be at that point the role of an operator. An operator, at that point, will have the possibility to configure this knob in conjunction with others. A possibility that Postgres is not currently giving. Postgres itself should be able to give this possibility, as these environments demand Postgres to address their emerging needs. Thank you, Gabriele -- Gabriele Bartolini Vice President, Cloud Native at EDB enterprisedb.com
Re: Correct the documentation for work_mem
On Mon, Sep 11, 2023 at 10:02:55PM +1200, David Rowley wrote: > On Sat, 9 Sept 2023 at 14:25, Imseih (AWS), Sami wrote: > > > > > This looks mostly fine to me modulo "sort or hash". I do see many > > > instances of "and/or" in the docs. Maybe that would work better. > > > > "sort or hash operations at the same time" is clear explanation IMO. > > Just for anyone else following along that haven't seen the patch. The > full text in question is: > > +Note that a complex query might perform several sort or hash > +operations at the same time, with each operation generally being > > It's certainly not a show-stopper. I do believe the patch makes some > improvements. The reason I'd prefer to see either "and" or "and/or" > in place of "or" is because the text is trying to imply that many of > these operations can run at the same time. I'm struggling to > understand why, given that there could be many sorts and many hashes > going on at once that we'd claim it could only be one *or* the other. > If we have 12 sorts and 4 hashes then that's not "several sort or hash > operations", it's "several sort and hash operations". Of course, it > could just be sorts or just hashes, so "and/or" works fine for that. Yes, I see your point and went with "and", updated patch attached. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you. diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 6bc1b215db..8ed7ae57c2 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1829,9 +1829,10 @@ include_dir 'conf.d' (such as a sort or hash table) before writing to temporary disk files. If this value is specified without units, it is taken as kilobytes. The default value is four megabytes (4MB). -Note that for a complex query, several sort or hash operations might be -running in parallel; each operation will generally be allowed -to use as much memory as this value specifies before it starts +Note that a complex query might perform several sort and hash +operations at the same time, with each operation generally being +allowed to use as much memory as this value specifies before +it starts to write data into temporary files. Also, several running sessions could be doing such operations concurrently. Therefore, the total memory used could be many times the value @@ -1845,7 +1846,7 @@ include_dir 'conf.d' Hash-based operations are generally more sensitive to memory availability than equivalent sort-based operations. The -memory available for hash tables is computed by multiplying +memory limit for a hash table is computed by multiplying work_mem by hash_mem_multiplier. This makes it possible for hash-based operations to use an amount of memory
Re: Possibility to disable `ALTER SYSTEM`
Greetings,
* Magnus Hagander (mag...@hagander.net) wrote:
> On Mon, Sep 11, 2023 at 1:56 PM Martín Marqués
> wrote:
> > > I would like to propose a patch that allows administrators to disable
> > > `ALTER SYSTEM` via either a runt-time option to pass to the Postgres
> > > server process at startup (e.g. `--disable-alter-system=true`, false by
> > > default) or a new GUC (or even both), without changing the current
> > > default method of the server.
> >
> > I'm actually going to put a strong +1 to Gabriele's proposal. It's an
> > undeniable problem (I'm only seeing arguments regarding other ways the
> > system would be insecure), and there might be real use cases for users
> > outside kubernetes for having this feature and using it (meaning
> > disabling the use of ALTER SYSTEM).
>
> If enough people are in favor of it *given the known issues with it*,
> I can drop my objection to a "meh, but I still don't think it's a good
> idea".
A lot of the objections seem to be on the grounds of returning a
'permission denied' kind of error and I generally agree with that being
the wrong approach.
As an alternative idea- what if we had something in postgresql.conf
along the lines of:
include_alter_system = true/false
and use that to determine if the postgresql.auto.conf is included or
not..?
> But to do that, there would need to be a very in-your-face warning in
> the documentation about it like "note that this only disables the
> ALTER SYSTEM command. It does not prevent a superuser from changing
> the configuration remotely using other means".
With the above, we could throw a WARNING or maybe just NOTICE when ALTER
SYSTEM is run that 'include_alter_system is false and therefore these
changes won't be included in the running configuration' or similar.
What this does cause problems with is that pg_basebackup and other tools
(eg: pgbackrest) write into postgresql.auto.conf currently and we'd want
those to still work. That's an opportunity, imv, though, since I don't
really think where ALTER SYSTEM writes to and where backup/restore
tools are writing to should really be the same place anyway. Therefore,
perhaps we add a 'postgresql.system.conf' or similar and maybe a
corresponding option in postgresql.conf to include it or not.
> For example, in the very simplest, wth the POC patch out there now, I
> can still run:
> postgres=# CREATE TEMP TABLE x(t text);
> CREATE TABLE
> postgres=# INSERT INTO x VALUES ('work_mem=1TB');
> INSERT 0 1
> postgres=# COPY x TO
> '/home/mha/postgresql/inst/head/data/postgresql.auto.conf';
> COPY 1
> postgres=# SELECT pg_reload_conf();
> pg_reload_conf
>
> t
> (1 row)
> postgres=# show work_mem;
> work_mem
> --
> 1TB
> (1 row)
>
> Or anything similar to that.
This is an issue if you're looking at it as a security thing. This
isn't an issue if don't view it that way. Still, I do see some merit in
making it so that you can't actually change the config that's loaded at
system start from inside the data directory or as the PG superuser,
which my proposal above would support- just configure in postgresql.conf
to not include any of the alter-system or generated config. The actual
postgresql.conf could be owned by root then too.
> > In Patroni for example, the PostgreSQL service is controlled on all
> > nodes by Patroni, and these kinds of changes could end up breaking the
> > cluster if there was a failover. For this reason Patroni starts
> > postgres with some GUC options as CMD arguments so that values in
> > postgresql.conf or postgresql.auto.conf are ignored. The values in the
> > DCS are the ones that matter.
>
> Right. And patroni would need to continue to do that even with this
> patch, because it also needs to override if somebody puts something in
> postgresql.conf, no? Removing the defence against that seems like a
> bad idea...
>
>
> > (see more about how Patroni manages this here:
> > https://patroni.readthedocs.io/en/latest/patroni_configuration.html#postgresql-parameters-controlled-by-patroni
> >
> > But let's face it, that's a hack, not something to be proud of, even
> > if it does what is intended. And this is a product and we shouldn't be
> > advertising hacks to overcome limitations.
>
> It's in a way a hack. But it's not the fault of ALTER SYSTEM, as you'd
> also have to manage postgresql.conf. One slightly less hacky part
> might be to have patroni generate a config file of it's own and
> include it with the highest priority -- at that point it *would* be
> come a hack around ALTER SYSTEM, because ALTER SYSTEM has a higher
> priority than any manual user config file. But it is not today.
I suppose we could invent a priority control thing as part of the above
proposal too.. but I would think just having include_alter_system and
include_auto_config (or whatever we name them) would be enough, with the
auto-config bit being loaded last and therefore having the 'highest'
priority.
> Another idea to solve the problem could
Re: How to add built-in func?
Hi,
> I only add below:
>
> Datum fake_dinstance2(PG_FUNCTION_ARGS)
> {
> PG_RETURN_INT16(0);
> }
> in src/backend/utils/adt/int8.c, and the I run “make install”,
> But I can’t find the fake_distance2 in src/backend/utils/fmgrtab.c which is
> generated by src/backend/utils/Gen_fmgrtab.pl. What else do I need to add?
If the goal is to add a function that can be executed by a user (e.g.
via psql) you have to add it to pg_proc.dat, or alternatively (and
often better) add a corresponding extension to /contrib/. You can find
a complete example here [1] for instance, see v4-0001 patch and the
function pg_get_relation_publishing_info(). Make sure it has a proper
volatility [2]. The patch [3] shows how to add an extension.
[1]:
https://postgr.es/m/CAAWbhmjcnoV7Xu6LHr_hxqWmVtehv404bvDye%2BQZcUDSg8NSKw%40mail.gmail.com
[2]: https://www.postgresql.org/docs/current/xfunc-volatility.html
[3]:
https://postgr.es/m/CAJ7c6TMSat6qjPrrrK0tRTgZsdXwFAbkDn5gjeDtFnUFrjZX-g%40mail.gmail.com
--
Best regards,
Aleksander Alekseev
Re: Possibility to disable `ALTER SYSTEM`
Hi Stephen, On Mon, 11 Sept 2023 at 17:12, Stephen Frost wrote: > A lot of the objections seem to be on the grounds of returning a > 'permission denied' kind of error and I generally agree with that being > the wrong approach. > > As an alternative idea- what if we had something in postgresql.conf > along the lines of: > > include_alter_system = true/false > > and use that to determine if the postgresql.auto.conf is included or > not..? > That sounds like a very good idea. I had thought about that when writing the PoC, as a SIGHUP controlled GUC. I had trouble finding an adequate GUC category for that (ideas?), and thought it was a more intrusive patch to trigger the conversation. But I am willing to explore that too (which won't change by any inch the goal of the patch). With the above, we could throw a WARNING or maybe just NOTICE when ALTER > SYSTEM is run that 'include_alter_system is false and therefore these > changes won't be included in the running configuration' or similar. > That's also an option I'd be willing to explore with folks here. > What this does cause problems with is that pg_basebackup and other tools > (eg: pgbackrest) write into postgresql.auto.conf currently and we'd want > those to still work. That's an opportunity, imv, though, since I don't > really think where ALTER SYSTEM writes to and where backup/restore > tools are writing to should really be the same place anyway. Therefore, > perhaps we add a 'postgresql.system.conf' or similar and maybe a > corresponding option in postgresql.conf to include it or not. > Totally. We are for example in the same position with the CloudNativePG operator, and it is something we are intending to fix ( https://github.com/cloudnative-pg/cloudnative-pg/issues/2727). I agree with you that it is the wrong place to do it. This is an issue if you're looking at it as a security thing. This > isn't an issue if don't view it that way. Still, I do see some merit in > making it so that you can't actually change the config that's loaded at > system start from inside the data directory or as the PG superuser, > which my proposal above would support- just configure in postgresql.conf > to not include any of the alter-system or generated config. The actual > postgresql.conf could be owned by root then too. > +1. Thank you, Gabriele -- Gabriele Bartolini Vice President, Cloud Native at EDB enterprisedb.com
Re: pg_rewind with cascade standby doesn't work well
Hi, > The attached patch updates minRecoveryPoint and minRecoveryPointTLI at this > point by mimicking CreateEndOfRecoveryRecord(). > With this patch, you can run pg_rewind with cascade standby immediately. > (without waiting for checkpointing) Many thanks for submitting the patch. I added it to the nearest open commitfest [1]. IMO a test is needed that makes sure no one is going to break this in the future. [1]: https://commitfest.postgresql.org/45/4559/ -- Best regards, Aleksander Alekseev
Re: Possibility to disable `ALTER SYSTEM`
On Mon, 11 Sept 2023 at 11:11, Magnus Hagander wrote: > I'm actually going to put a strong +1 to Gabriele's proposal. It's an > > undeniable problem (I'm only seeing arguments regarding other ways the > > system would be insecure), and there might be real use cases for users > > outside kubernetes for having this feature and using it (meaning > > disabling the use of ALTER SYSTEM). > > If enough people are in favor of it *given the known issues with it*, > I can drop my objection to a "meh, but I still don't think it's a good > idea". > > But to do that, there would need to be a very in-your-face warning in > the documentation about it like "note that this only disables the > ALTER SYSTEM command. It does not prevent a superuser from changing > the configuration remotely using other means". > > For example, in the very simplest, wth the POC patch out there now, I > can still run: > […] Maybe in addition to making "ALTER SYSTEM" throw an error, the feature that disables it should also disable reading postgresql.auto.conf? Maybe even delete it and make it an error if it is present on startup (maybe even warn if it shows up while the DB is running?). Interesting corner case: What happens if I do "ALTER SYSTEM SET alter_system_disabled = true"? Counterpoint: maybe the idea is to disable ALTER SYSTEM but still use postgresql.auto.conf, maintained by an external program, to control the instance's behaviour.
Re: How to add built-in func?
Hi
po 11. 9. 2023 v 17:59 odesílatel jacktby jacktby
napsal:
> I only add below:
>
> Datum fake_dinstance2(PG_FUNCTION_ARGS)
> {
> PG_RETURN_INT16(0);
> }
> in src/backend/utils/adt/int8.c, and the I run “make install”,
> But I can’t find the fake_distance2 in src/backend/utils/fmgrtab.c which is
> generated by src/backend/utils/Gen_fmgrtab.pl. What else do I need to add?
>
you need to add the function metadata to pg_proc.dat
For free oid use unused_oids script
Regards
Pavel
Re: How to add built-in func?
> 2023年9月11日 23:51，Aleksander Alekseev 写道：
>
> Hi,
>
>> I only add below:
>>
>> Datum fake_dinstance2(PG_FUNCTION_ARGS)
>> {
>>PG_RETURN_INT16(0);
>> }
>> in src/backend/utils/adt/int8.c, and the I run “make install”,
>> But I can’t find the fake_distance2 in src/backend/utils/fmgrtab.c which is
>> generated by src/backend/utils/Gen_fmgrtab.pl. What else do I need to add?
>
> If the goal is to add a function that can be executed by a user (e.g.
> via psql) you have to add it to pg_proc.dat, or alternatively (and
> often better) add a corresponding extension to /contrib/. You can find
> a complete example here [1] for instance, see v4-0001 patch and the
> function pg_get_relation_publishing_info(). Make sure it has a proper
> volatility [2]. The patch [3] shows how to add an extension.
>
> [1]:
> https://postgr.es/m/CAAWbhmjcnoV7Xu6LHr_hxqWmVtehv404bvDye%2BQZcUDSg8NSKw%40mail.gmail.com
> [2]: https://www.postgresql.org/docs/current/xfunc-volatility.html
> [3]:
> https://postgr.es/m/CAJ7c6TMSat6qjPrrrK0tRTgZsdXwFAbkDn5gjeDtFnUFrjZX-g%40mail.gmail.com
> --
> Best regards,
> Aleksander Alekseev
I need to make it used for a new operator in my pg.
RE: [PoC] pg_upgrade: allow to upgrade publisher node
Dear Amit,
Thank you for reviewing!
> Few comments:
> ==
> 1.
> +linkend="view-pg-replication-slots">pg_replication_slots.c
> onfirmed_flush_lsn
> + of all slots on the old cluster must be the same as the latest
> + checkpoint location.
>
> We can add something like: "This ensures that all the data has been
> replicated before the upgrade." to make it clear why this test is
> important.
Added.
> 2. Move the wal_level related restriction before max_replication_slots.
>
> 3.
> + /* Is the slot still usable? */
> + if (slot->invalid)
> + {
> + if (script == NULL &&
> + (script = fopen_priv(output_path, "w")) == NULL)
> + pg_fatal("could not open file \"%s\": %s",
> + output_path, strerror(errno));
> +
> + fprintf(script,
> + "slotname :%s\tproblem: The slot is unusable\n",
> + slot->slotname);
> + }
> +
> + /*
> + * Do additional checks to ensure that confirmed_flush LSN of all
> + * the slots is the same as the latest checkpoint location.
> + *
> + * Note: This can be satisfied only when the old cluster has been
> + * shut down, so we skip this for live checks.
> + */
> + if (!live_check && !slot->caught_up)
>
> Isn't it better to continue for the next slot once we find that slot
> is invalid instead of checking other conditions?
Right, fixed.
> 4.
> +
> + fprintf(script,
> + "slotname :%s\tproblem: The slot is unusable\n",
> + slot->slotname);
>
> Let's keep it as one string and change the message to: "The slot
> "\"%s\" is invalid"
Changed.
> + fprintf(script,
> + "slotname :%s\tproblem: The slot has not consumed WALs yet\n",
> + slot->slotname);
> + }
>
> On a similar line, we can change this to: "The slot "\"%s\" has not
> consumed the WAL yet"
Changed.
> 5.
> + snprintf(output_path, sizeof(output_path), "%s/%s",
> + log_opts.basedir,
> + "problematic_logical_relication_slots.txt");
>
> I think we can name this file as "invalid_logical_replication_slots"
> or simply "logical_replication_slots"
The latter one seems too general for me, "invalid_..." was chosen.
> 6.
> + pg_fatal("The source cluster contains one or more problematic
> logical replication slots.\n"
> + "The needed workaround depends on the problem.\n"
> + "1) If the problem is \"The slot is unusable,\" You can drop such
> replication slots.\n"
> + "2) If the problem is \"The slot has not consumed WALs yet,\" you
> can consume all remaining WALs.\n"
> + "Then, you can restart the upgrade.\n"
> + "A list of problematic logical replication slots is in the file:\n"
> + "%s", output_path);
>
> This doesn't match the similar existing comments. So, let's change it
> to something like:
>
> "Your installation contains invalid logical replication slots. These
> slots can't be copied so this cluster cannot currently be upgraded.
> Consider either removing such slots or consuming the pending WAL if
> any and then restart the upgrade. A list of invalid logical
> replication slots is in the file:"
Basically changed to your suggestion, but slightly reworded based on
what Grammarly said.
> Apart from the above, I have edited a few other comments in the patch.
> See attached.
Thanks for attaching! Included.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Re: How to add built-in func?
On 2023-09-11 12:28, jacktby jacktby wrote: 2023年9月11日 23:51，Aleksander Alekseev 写道： often better) add a corresponding extension to /contrib/. You can find a complete example here [1] for instance, see v4-0001 patch and the function pg_get_relation_publishing_info(). Make sure it has a proper volatility [2]. The patch [3] shows how to add an extension. [1]: https://postgr.es/m/CAAWbhmjcnoV7Xu6LHr_hxqWmVtehv404bvDye%2BQZcUDSg8NSKw%40mail.gmail.com [2]: https://www.postgresql.org/docs/current/xfunc-volatility.html [3]: https://postgr.es/m/CAJ7c6TMSat6qjPrrrK0tRTgZsdXwFAbkDn5gjeDtFnUFrjZX-g%40mail.gmail.com -- I need to make it used for a new operator in my pg. You can implement both a function and an operator (and all that goes with) in an extension, without having to hack at all on PostgreSQL itself. You can then, if it seems generally useful enough, offer that extension to go in contrib/. If it's agreed to be something everyone should have, it could then make its way into core. Do you have it working as an extension yet? That can be a good way to start, separating the difficulties you have to solve from the ones you don't have to solve yet. Regards, -Chap
Re: How to add built-in func?
po 11. 9. 2023 v 18:18 odesílatel Pavel Stehule
napsal:
> Hi
>
> po 11. 9. 2023 v 17:59 odesílatel jacktby jacktby
> napsal:
>
>> I only add below:
>>
>> Datum fake_dinstance2(PG_FUNCTION_ARGS)
>> {
>> PG_RETURN_INT16(0);
>> }
>> in src/backend/utils/adt/int8.c, and the I run “make install”,
>> But I can’t find the fake_distance2 in src/backend/utils/fmgrtab.c which
>> is
>> generated by src/backend/utils/Gen_fmgrtab.pl. What else do I need to
>> add?
>>
>
> you need to add the function metadata to pg_proc.dat
>
> For free oid use unused_oids script
>
https://www.postgresql.org/docs/current/system-catalog-initial-data.html
https://www.highgo.ca/2021/03/04/how-to-create-a-system-information-function-in-postgresql/
> Regards
>
> Pavel
>
>
Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)
On Mon, 2023-09-11 at 07:24 +0900, Michael Paquier wrote: > I'm OK with enforcing COLLPROVIDER_LIBC in this path, but I also > value > consistency across all the error messages of this file. After > sleeping on it, and as that's a set of elogs, "unsupported > collprovider" is fine for me across the board as these should not be > user-visible. That's fine with me. Regards, Jeff Davis
Re: proposal: psql: show current user in prompt
po 11. 9. 2023 v 13:24 odesílatel Jelte Fennema napsal:
> On Fri, 8 Sept 2023 at 21:08, Pavel Stehule
> wrote:
> > ok changed - there is minor problem - almost all PQ functions are of int
> type, but ProtocolVersion is uint
>
> Your current implementation requires using the PG_PROTOCOL macros to
> compare versions. But clients cannot currently use those macros since
> they are not exported from libpq-fe.h, only from pqcomm.h which is
> then imported by libpq-int.h. (psql/command.c imports pcomm.h
> directly, but I don't think we should expect clients to do that). We
> could ofcourse export these macros from libpq-fe.h too. But then we'd
> need to document those macros too.
>
> > Using different mapping to int can be problematic - can be messy if we
> cannot use PG_PROTOCOL macro.
>
> I see no big problems returning an unsigned or long from the new
> function (even if existing functions all returned int). But I don't
> even think that is necessary. Returning the following as an int from
> PQprotocolVersionFull would work fine afaict:
>
> return PG_PROTOCOL_MAJOR(version) * 1000 + PG_PROTOCOL_MINOR(version)
>
> This would give us one thousand minor versions for each major version.
> This seems fine for all practical purposes. Since postgres only
> releases a version once every year, we'd need a protocol bump every
> year for one thousand years for that to ever cause any problems. So
> I'd prefer this approach over making the PG_PROTOCOL macros a public
> interface.
>
I did proposed change
Regards
Pavel
From 2f432cbb6d38569759e12a2cbe80ec9dce4b2f25 Mon Sep 17 00:00:00 2001
From: "ok...@github.com"
Date: Mon, 24 Jul 2023 20:18:16 +0200
Subject: [PATCH 4/4] Implementation of %N prompt placeholder
It is based on forcing reporting feature"role" GUC to client.
---
doc/src/sgml/ref/psql-ref.sgml | 19 ++-
src/bin/psql/command.c | 13 +
src/bin/psql/prompt.c | 27 +++
src/bin/psql/settings.h| 1 +
src/bin/psql/startup.c | 32
5 files changed, 91 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index d94e3cacfc..8b267a6da6 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4568,7 +4568,24 @@ 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 set to the database session user name.
+
+
+
+ This substitution requires PostgreSQL
+ version 16 and up. When you use older version, the empty string
+ is used instead.
+
+
+
+
+
%n
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index bcd8eb3538..bad0fdf415 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3883,6 +3883,7 @@ SyncVariables(void)
{
char vbuf[32];
const char *server_version;
+ PGresult *result;
/* get stuff from connection */
pset.encoding = PQclientEncoding(pset.db);
@@ -3912,6 +3913,18 @@ SyncVariables(void)
/* send stuff to it, too */
PQsetErrorVerbosity(pset.db, pset.verbosity);
PQsetErrorContextVisibility(pset.db, pset.show_context);
+
+ /* link role GUC when it is needed for prompt */
+ if (pset.prompt_shows_role)
+ result = PQlinkParameterStatus(pset.db, "role");
+ else
+ result = PQunlinkParameterStatus(pset.db, "role");
+
+ if (PQresultStatus(result) != PGRES_COMMAND_OK)
+ pg_log_info("cannot set REPORT flag on configuration variable \"role\": %s",
+ PQerrorMessage(pset.db));
+
+ PQclear(result);
}
/*
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e..566b47d814 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -165,6 +165,33 @@ 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)
+ {
+ const char *rolename = NULL;
+
+ /*
+ * This feature requires GUC "role" to be marked
+ * by GUC_REPORT flag. This is done by PQlinkParameterStatus
+ * function. This function requires protocol 3.1 (ReportGUC
+ * message). Fallback is empty string.
+ */
+ if (PQprotocolVersionFull(pset.db) >= PQmakeProtocolVersionFull(3,1))
+ {
+ rolename = PQparameterStatus(pset.db, "role");
+
+ /* fallback when role is not set yet */
+ if (rolename && strcmp(rolename, "none") == 0)
+rolename = session_username();
+ }
+
+ if (rolename)
+ strlcpy(buf, rolename, sizeof(buf));
+ else
+ buf[0] = '\0';
+ }
+ break;
/* backend pid
Re: Query execution in Perl TAP tests needs work
On Sun, Sep 3, 2023 at 12:17 PM Thomas Munro wrote: > (Huh, while contemplating trying that, I just noticed that the GCC > build farm's AIX 7.2 system seems to have given up the ghost a few > weeks ago. I wonder if it'll come back online with the current > release, or if that's the end... There is still the > overloaded-to-the-point-of-being-hard-to-interact-with AIX 7.1 (=EOL) > machine.) FTR it (gcc119) appears to have come back online, now upgraded to AIX 7.3. No reports from "hoverfly" (I think it was on that host?). It probably needs some attention to start working again after the upgrade.
Re: lockup in parallel hash join on dikkop (freebsd 14.0-current)
On Sat, Sep 9, 2023 at 9:00 PM Alexander Lakhin wrote: > Yes, I think we deal with something like that. I can try to deduce a minimum > change that affects reproducing the issue, but may be it's not that important. > Perhaps we now should think of escalating the problem to FreeBSD developers? > I wonder, what kind of reproducer they find acceptable. A standalone C > program only or maybe a script that compiles/installs postgres and runs > our test will do too? We discussed this a bit off-list and I am following up on that. My guess is that this will turn out to be a bad interaction between that optimisation and our (former) habit of forking background workers from inside a signal handler, but let's see... FTR If someone is annoyed by this and just wants their build farm animal not to hang on REL_12_STABLE, via Alexander's later experiments we learned that sysctl kern.sigfastblock_fetch_always=1 fixes the problem.
Re: Row pattern recognition
On Sat, Sep 9, 2023 at 4:21 AM Tatsuo Ishii wrote: > Then we will get for str_set: > r0: B > r1: AB > > Because r0 only has classifier B, r1 can have A and B. Problem is, > r2. If we choose A at r1, then r2 = B. But if we choose B at t1, then > r2 = AB. I guess this is the issue you pointed out. Right. > Yeah, probably we have delay evaluation of such pattern variables like > A, then reevaluate A after the first scan. > > What about leaving this (reevaluation) for now? Because: > > 1) we don't have CLASSIFIER > 2) we don't allow to give CLASSIFIER to PREV as its arggument > > so I think we don't need to worry about this for now. Sure. I'm all for deferring features to make it easier to iterate; I just want to make sure the architecture doesn't hit a dead end. Or at least, not without being aware of it. Also: is CLASSIFIER the only way to run into this issue? > What if we don't follow the standard, instead we follow POSIX EREs? I > think this is better for users unless RPR's REs has significant merit > for users. Piggybacking off of what Vik wrote upthread, I think we would not be doing ourselves any favors by introducing a non-compliant implementation that performs worse than a traditional NFA. Those would be some awful bug reports. > > - I think we have to implement a parallel parser regardless (RPR PATTERN > > syntax looks incompatible with POSIX) > > I am not sure if we need to worry about this because of the reason I > mentioned above. Even if we adopted POSIX NFA semantics, we'd still have to implement our own parser for the PATTERN part of the query. I don't think there's a good way for us to reuse the parser in src/backend/regex. > > Does that seem like a workable approach? (Worst-case, my code is just > > horrible, and we throw it in the trash.) > > Yes, it seems workable. I think for the first cut of RPR needs at > least the +quantifier with reasonable performance. The current naive > implementation seems to have issue because of exhaustive search. +1 Thanks! --Jacob
Re: [PoC] pg_upgrade: allow to upgrade publisher node
Hi Kuroda-san, here are my review comments for v34-0002
There is likely to be some overlap because others have modified and/or
commented on some of the same points as me, and v35 was already posted
before this review. I'll leave it to you to sort out any clashes and
ignore them where appropriate.
==
1. GENERAL -- Cluster Terminology
This is not really a problem of your patch, but during message review,
I noticed the terms old/new cluster VERSUS source/target cluster and
both were used many times:
For example.
".*new clusmter --> 44 occurences
".*old cluster --> 21 occurences
".*source cluster --> 6 occurences
".*target cluster --> 12 occurences
Perhaps there should be a new thread/patch to use consistent terms.
Thoughts?
~~~
2. GENERAL - Error message cases
Just FYI, there are many inconsistent capitalising in these patch
messages, but then the same is also true for the HEAD code. It's a bit
messy, but generally, I think your capitalisation was aligned with
what I saw in HEAD, so I didn't comment anywhere about it.
==
src/backend/replication/slot.c
3. InvalidatePossiblyObsoleteSlot
+ /*
+ * Raise an ERROR if the logical replication slot is invalidating. It
+ * would not happen because max_slot_wal_keep_size is set to -1 during
+ * the upgrade, but it stays safe.
+ */
+ if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade)
+ elog(ERROR, "Replication slots must not be invalidated during the upgrade.");
3a.
That comment didn't seem good. I think you mean like in the suggestion below.
SUGGESTION
It should not be possible for logical replication slots to be
invalidated because max_slot_wal_keep_size is set to -1 during the
upgrade. The following is just for sanity-checking.
~
3b.
I wasn't sure if 'max_slot_wal_keep_size' GUC is accessible in this
scope, but if it is available then maybe
Assert(max_slot_wal_keep_size_mb == -1); should also be included in
this sanity check.
==
src/bin/pg_upgrade/check.c
4. check_new_cluster_logical_replication_slots
+ conn = connectToServer(&new_cluster, "template1");
+
+ prep_status("Checking for logical replication slots");
There is some inconsistency with all the subsequent pg_fatals within
this function -- some of them mention "New cluster" but most of them
do not.
Meanwhile, Kuroda-san showed me sample output like:
Checking for presence of required libraries ok
Checking database user is the install userok
Checking for prepared transactionsok
Checking for new cluster tablespace directories ok
Checking for logical replication slots
New cluster must not have logical replication slots but found 1 slot.
Failure, exiting
So, I felt the log message title ("Checking...") should be changed to
include the words "new cluster" just like the log preceding it:
"Checking for logical replication slots" ==> "Checking for new cluster
logical replication slots"
Now all the subsequent pg_fatals clearly are for "new cluster"
~
5. check_new_cluster_logical_replication_slots
+ if (nslots_on_new)
+ pg_fatal(ngettext("New cluster must not have logical replication
slots but found %d slot.",
+ "New cluster must not have logical replication slots but found %d slots.",
+ nslots_on_new),
+ nslots_on_new);
5a.
TBH, I didn't see why you go to unnecessary trouble to have a plural
message here. The message could just be like:
"New cluster must have 0 logical replication slots but found %d."
~
5b.
However, now (from the previous review comment #4) if "New cluster" is
already explicit in the log, the pg_fatal message can become just:
"New cluster must have ..." ==> "Expected 0 logical replication slots
but found %d."
~~~
6. check_old_cluster_for_valid_slots
+ if (script)
+ {
+ fclose(script);
+
+ pg_log(PG_REPORT, "fatal");
+ pg_fatal("The source cluster contains one or more problematic
logical replication slots.\n"
+ "The needed workaround depends on the problem.\n"
+ "1) If the problem is \"The slot is unusable,\" You can drop such
replication slots.\n"
+ "2) If the problem is \"The slot has not consumed WALs yet,\" you
can consume all remaining WALs.\n"
+ "Then, you can restart the upgrade.\n"
+ "A list of problematic logical replication slots is in the file:\n"
+ "%s", output_path);
+ }
This needs fixing but I saw it has been updated in v35, so I'll check
it there later.
==
src/bin/pg_upgrade/info.c
7. get_db_rel_and_slot_infos
void
get_db_rel_and_slot_infos(ClusterInfo *cluster)
{
int dbnum;
if (cluster->dbarr.dbs != NULL)
free_db_and_rel_infos(&cluster->dbarr);
~
Judging from the HEAD code this function was intended to be reentrant
-- e.g. it does cleanup code free_db_and_rel_infos in case there was
something there from before.
IIUC there is no such cleanup for the slot_arr. I forget why this was
removed. Sure, you might be able to survive the memory leaks, but
choosing NOT to clean up the slot_arr seems to contradict the
intention of HEAD calling free_db_an
Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)
On Mon, Sep 11, 2023 at 12:15:49PM -0700, Jeff Davis wrote:
> That's fine with me.
Okay. Then, please find attached a v4 for HEAD and REL_16_STABLE.
--
Michael
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index aa9da99308..c02fa7b203 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -2031,7 +2031,8 @@ pg_strcoll(const char *arg1, const char *arg2, pg_locale_t locale)
#endif
else
/* shouldn't happen */
- elog(ERROR, "unsupported collprovider: %c", locale->provider);
+ elog(ERROR, "unsupported collprovider (%s): %c",
+ "pg_strcoll", locale->provider);
return result;
}
@@ -2067,7 +2068,8 @@ pg_strncoll(const char *arg1, size_t len1, const char *arg2, size_t len2,
#endif
else
/* shouldn't happen */
- elog(ERROR, "unsupported collprovider: %c", locale->provider);
+ elog(ERROR, "unsupported collprovider (%s): %c",
+ "pg_strncoll", locale->provider);
return result;
}
@@ -2086,7 +2088,8 @@ pg_strxfrm_libc(char *dest, const char *src, size_t destsize,
return strxfrm(dest, src, destsize);
#else
/* shouldn't happen */
- elog(ERROR, "unsupported collprovider: %c", locale->provider);
+ elog(ERROR, "unsupported collprovider (%s): %c",
+ "pg_strxfrm_libc", locale->provider);
return 0; /* keep compiler quiet */
#endif
}
@@ -2282,7 +2285,8 @@ pg_strxfrm_enabled(pg_locale_t locale)
return true;
else
/* shouldn't happen */
- elog(ERROR, "unsupported collprovider: %c", locale->provider);
+ elog(ERROR, "unsupported collprovider (%s): %c",
+ "pg_strxfrm_enabled", locale->provider);
return false;/* keep compiler quiet */
}
@@ -2314,7 +2318,8 @@ pg_strxfrm(char *dest, const char *src, size_t destsize, pg_locale_t locale)
#endif
else
/* shouldn't happen */
- elog(ERROR, "unsupported collprovider: %c", locale->provider);
+ elog(ERROR, "unsupported collprovider (%s): %c",
+ "pg_strxfrm", locale->provider);
return result;
}
@@ -2351,7 +2356,8 @@ pg_strnxfrm(char *dest, size_t destsize, const char *src, size_t srclen,
#endif
else
/* shouldn't happen */
- elog(ERROR, "unsupported collprovider: %c", locale->provider);
+ elog(ERROR, "unsupported collprovider (%s): %c",
+ "pg_strnxfrm", locale->provider);
return result;
}
@@ -2369,7 +2375,8 @@ pg_strxfrm_prefix_enabled(pg_locale_t locale)
return true;
else
/* shouldn't happen */
- elog(ERROR, "unsupported collprovider: %c", locale->provider);
+ elog(ERROR, "unsupported collprovider (%s): %c",
+ "pg_strxfrm_prefix_enabled", locale->provider);
return false;/* keep compiler quiet */
}
@@ -2393,17 +2400,16 @@ pg_strxfrm_prefix(char *dest, const char *src, size_t destsize,
{
size_t result = 0; /* keep compiler quiet */
- if (!locale || locale->provider == COLLPROVIDER_LIBC)
- elog(ERROR, "collprovider '%c' does not support pg_strxfrm_prefix()",
- locale->provider);
+ if (!locale)
+ elog(ERROR, "unsupported collprovider (%s): %c",
+ "pg_strxfrm_prefix", COLLPROVIDER_LIBC);
#ifdef USE_ICU
else if (locale->provider == COLLPROVIDER_ICU)
result = pg_strnxfrm_prefix_icu(dest, src, -1, destsize, locale);
#endif
else
- /* shouldn't happen */
- elog(ERROR, "unsupported collprovider: %c", locale->provider);
-
+ elog(ERROR, "unsupported collprovider (%s): %c",
+ "pg_strxfrm_prefix", locale->provider);
return result;
}
@@ -2430,16 +2436,16 @@ pg_strnxfrm_prefix(char *dest, size_t destsize, const char *src,
{
size_t result = 0; /* keep compiler quiet */
- if (!locale || locale->provider == COLLPROVIDER_LIBC)
- elog(ERROR, "collprovider '%c' does not support pg_strnxfrm_prefix()",
- locale->provider);
+ if (!locale)
+ elog(ERROR, "unsupported collprovider (%s): %c",
+ "pg_strnxfrm_prefix", COLLPROVIDER_LIBC);
#ifdef USE_ICU
else if (locale->provider == COLLPROVIDER_ICU)
result = pg_strnxfrm_prefix_icu(dest, src, -1, destsize, locale);
#endif
else
- /* shouldn't happen */
- elog(ERROR, "unsupported collprovider: %c", locale->provider);
+ elog(ERROR, "unsupported collprovider (%s): %c",
+ "pg_strnxfrm_prefix", locale->provider);
return result;
}
signature.asc
Description: PGP signature
Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)
Em seg., 11 de set. de 2023 às 21:03, Michael Paquier escreveu: > On Mon, Sep 11, 2023 at 12:15:49PM -0700, Jeff Davis wrote: > > That's fine with me. > > Okay. Then, please find attached a v4 for HEAD and REL_16_STABLE. > LGTM. best regards, Ranier Vilela
Re: [PoC] pg_upgrade: allow to upgrade publisher node
Hi Kuroda-san. Here are some additional review comments for v35-0002 (and because we overlapped, my v34-0002 review comments have not been addressed yet) == Commit message 1. Note that the pg_resetwal command would remove WAL files, which are required as restart_lsn. If WALs required by logical replication slots are removed, they are unusable. Therefore, during the upgrade, slot restoration is done after the final pg_resetwal command. The workflow ensures that required WALs are remained. ~ SUGGESTION (minor wording and /required as/required for/ and /remained/retained/) Note that the pg_resetwal command would remove WAL files, which are required for restart_lsn. If WALs required by logical replication slots are removed, the slots are unusable. Therefore, during the upgrade, slot restoration is done after the final pg_resetwal command. The workflow ensures that required WALs are retained. == doc/src/sgml/ref/pgupgrade.sgml 2. The SGML is mal-formed so I am unable to build PG DOCS. Please try building the docs before posting the patch. ref/pgupgrade.sgml:446: parser error : Opening and ending tag mismatch: itemizedlist line 410 and listitem ^ ~~~ 3. + + + The new cluster must not have permanent logical replication slots, i.e., + there are no slots whose + pg_replication_slots.temporary + is false. + + /there are no slots whose/there must be no slots where/ ~~~ 4. or take a file system backup as the standbys are still synchronized - with the primary.) Replication slots are not copied and must - be recreated. + with the primary.) Only logical slots on the primary are migrated to the + new standby, and other slots on the old standby must be recreated as + they are not copied. Mixing the terms "migrated" and "copied" seems to complicate this. Does the following suggestion work better instead? SUGGESTION (??) Only logical slots on the primary are migrated to the new standby. Any other slots present on the old standby must be recreated. == src/backend/replication/slot.c 5. InvalidatePossiblyObsoleteSlot + /* + * The logical replication slots shouldn't be invalidated as + * max_slot_wal_keep_size is set to -1 during the upgrade. + */ + if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade) + elog(ERROR, "Replication slots must not be invalidated during the upgrade."); + I felt the comment could have another sentence like "The following is just a sanity check." == src/bin/pg_upgrade/function.c 6. get_loadable_libraries + array_size = totaltups + count_old_cluster_logical_slots(); + os_info.libraries = (LibraryInfo *) pg_malloc(sizeof(LibraryInfo) * (array_size)); totaltups = 0; 6a. Maybe something like 'n_libinfos' would be a more meaningful name than 'array_size'? ~ 6b. + os_info.libraries = (LibraryInfo *) pg_malloc(sizeof(LibraryInfo) * (array_size)); Those extra parentheses around "(array_size)" seem overkill. -- Kind Regards, Peter Smith. Fujitsu Australia
Re: broken master regress tests
Hi,
On 2023-08-29 17:54:24 +0200, Alvaro Herrera wrote:
> On 2023-Aug-27, Thomas Munro wrote:
>
> > On Sun, Aug 27, 2023 at 3:03 AM Pavel Stehule
> > wrote:
> > > So it looks so IPC::Run::run is ignore parent environment
> >
> > I guess the new initdb template captures lc_messages in
> > postgresql.conf, when it runs earlier? I guess if you put
> > $node->append_conf('postgresql.conf', 'lc_messages=C'); into
> > src/bin/pg_amcheck/t/003_check.pl then it will work. I'm not sure
> > what the correct fix should be, ie if the template mechanism should
> > notice this difference and not use the template, or if tests that
> > depend on the message locale should explicitly say so with
> > lc_messages=C or similar (why is this the only one?), or ...
>
> So I tried this technique, but it gest old pretty fast: apparently
> there's a *ton* of tests that depend on the locale. I gave up after
> patching the first five files, and noticing that in a second run there
> another half a dozen failing tests that hadn't failed the first time
> around. (Not sure why this happened.)
>
> So I think injecting --no-locale to the initdb line that creates the
> template is a better approach; something like the attached.
Makes sense, thanks for taking care of this.
Greetings,
Andres Freund
RE: [PoC] pg_upgrade: allow to upgrade publisher node
On Monday, September 11, 2023 9:22 PM Kuroda, Hayato/黒田 隼人 wrote: > > Thank you for reviewing! PSA new version. Thanks for updating the patch, few cosmetic comments: 1. #include "access/transam.h" #include "catalog/pg_language_d.h" +#include "fe_utils/string_utils.h" #include "pg_upgrade.h" It seems we don't need this head file anymore. 2. + if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade) + elog(ERROR, "Replication slots must not be invalidated during the upgrade."); I think normally the first letter is lowercase, and we can avoid the period. Best Regards, Hou zj
Re: PSQL error: total cell count of XXX exceeded
Thank you for your advice, Jelte. I have refactored my code, please see the attached patch. (and I put it into https://commitfest.postgresql.org/45/ for trace) Thanks. From: Jelte Fennema Sent: Monday, September 11, 2023 15:04 To: Hongxu Ma Cc: David G. Johnston ; PostgreSQL Hackers Subject: Re: PSQL error: total cell count of XXX exceeded On Mon, 11 Sept 2023 at 08:51, Hongxu Ma wrote: > > I created a patch to fix it. > Really appreciate to anyone can help to review it. > Thanks. I think "product" as a variable name isn't very descriptive. Let's call it total_cells (or something similar instead). Other than that I think it's a good change. content->cellsadded is also a long, So I agree that I don't think the limit of int cells was intended here. v2-0001-Using-long-type-in-printTableAddCell.patch Description: v2-0001-Using-long-type-in-printTableAddCell.patch
Re: Add 'worker_type' to pg_stat_subscription
On Fri, Sep 8, 2023 at 8:28 AM Nathan Bossart wrote: > > On Thu, Sep 07, 2023 at 12:36:29PM +1200, Peter Smith wrote: > > Modified as suggested. PSA v3. > > Thanks. I've attached v4 with a couple of small changes. Notably, I've > moved the worker_type column to before the pid column, as it felt more > natural to me to keep the PID columns together. I've also added an > elog(ERROR, ...) for WORKERTYPE_UNKNOWN, as that seems to be the standard > practice elsewhere. > That being said, are we absolutely confident that this > really cannot happen? I haven't looked too closely, but if there is a > small race or something that could cause us to see a worker with this type, > perhaps it would be better to actually list it as "unknown". Thoughts? The type is only assigned during worker process launch, and during process cleanup [1]. It's only possible to be UNKNOWN after logicalrep_worker_cleanup(). AFAIK the stats can never see a worker with an UNKNOWN type, although it was due to excessive caution against something unforeseen that my original code did below instead of the elog. + case WORKERTYPE_UNKNOWN: /* should not be possible */ + nulls[9] = true; Adding "unknown" for something that is supposed to be impossible might be slight overkill, but so long as there is no obligation to write about "unknown" in the PG DOCS then I agree it is probably better to do that, -- [1] https://github.com/search?q=repo%3Apostgres%2Fpostgres%20%20worker-%3Etype&type=code Kind Regards, Peter Smith. Fujitsu Australia
Re: Add 'worker_type' to pg_stat_subscription
On Tue, Sep 12, 2023 at 01:07:51PM +1000, Peter Smith wrote: > The type is only assigned during worker process launch, and during > process cleanup [1]. It's only possible to be UNKNOWN after > logicalrep_worker_cleanup(). > > AFAIK the stats can never see a worker with an UNKNOWN type, although > it was due to excessive caution against something unforeseen that my > original code did below instead of the elog. > > + case WORKERTYPE_UNKNOWN: /* should not be possible */ > + nulls[9] = true; > > Adding "unknown" for something that is supposed to be impossible might > be slight overkill, but so long as there is no obligation to write > about "unknown" in the PG DOCS then I agree it is probably better to > do that, Using an elog() is OK IMO. pg_stat_get_subscription() holds LogicalRepWorkerLock in shared mode, and the only code path setting a worker to WORKERTYPE_UNKNOWN requires that this same LWLock is hold in exclusive mode while resetting all the shmem fields of the subscription entry cleaned up, which is what pg_stat_get_subscription() uses to check if a subscription should be included in its SRF. Shouldn't this patch add or tweak some SQL queries in src/test/subscription/ to show some worker types, at least? -- Michael signature.asc Description: PGP signature
Re: PSQL error: total cell count of XXX exceeded
On Tue, Sep 12, 2023 at 02:39:55AM +, Hongxu Ma wrote:
> Thank you for your advice, Jelte.
> I have refactored my code, please see the attached patch. (and I put
> it into https://commitfest.postgresql.org/45/ for trace)
{
+ long total_cells;
long is 4 bytes on Windows, and 8 bytes basically elsewhere. So you
would still have the same problem on Windows, no?
--
Michael
signature.asc
Description: PGP signature
Re: [PoC] pg_upgrade: allow to upgrade publisher node
On Tue, Sep 12, 2023 at 02:33:25AM +, Zhijie Hou (Fujitsu) wrote: > 2. > + if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade) > + elog(ERROR, "Replication slots must not be invalidated > during the upgrade."); > > I think normally the first letter is lowercase, and we can avoid the period. Documentation is your friend: https://www.postgresql.org/docs/current/error-style-guide.html -- Michael signature.asc Description: PGP signature
Re: PSQL error: total cell count of XXX exceeded
Michael Paquier writes: > On Tue, Sep 12, 2023 at 02:39:55AM +, Hongxu Ma wrote: > + long total_cells; > long is 4 bytes on Windows, and 8 bytes basically elsewhere. So you > would still have the same problem on Windows, no? More to the point: what about the multiplication in printTableInit? The cat's been out of the bag for quite some time before we get to printTableAddCell. I'm more than a bit skeptical about trying to do something about this, simply because this range of query result sizes is far past what is practical. The OP clearly hasn't tested his patch on actually overflowing query results, and I don't care to either. regards, tom lane
Re: How to add built-in func?
> 2023年9月12日 00:34，Chapman Flack 写道： > > On 2023-09-11 12:28, jacktby jacktby wrote: >>> 2023年9月11日 23:51，Aleksander Alekseev 写道： >>> often better) add a corresponding extension to /contrib/. You can find >>> a complete example here [1] for instance, see v4-0001 patch and the >>> function pg_get_relation_publishing_info(). Make sure it has a proper >>> volatility [2]. The patch [3] shows how to add an extension. >>> [1]: >>> https://postgr.es/m/CAAWbhmjcnoV7Xu6LHr_hxqWmVtehv404bvDye%2BQZcUDSg8NSKw%40mail.gmail.com >>> [2]: https://www.postgresql.org/docs/current/xfunc-volatility.html >>> [3]: >>> https://postgr.es/m/CAJ7c6TMSat6qjPrrrK0tRTgZsdXwFAbkDn5gjeDtFnUFrjZX-g%40mail.gmail.com >>> -- >> I need to make it used for a new operator in my pg. > > You can implement both a function and an operator (and all that goes with) > in an extension, without having to hack at all on PostgreSQL itself. > You can then, if it seems generally useful enough, offer that extension > to go in contrib/. If it's agreed to be something everyone should have, > it could then make its way into core. > > Do you have it working as an extension yet? That can be a good way > to start, separating the difficulties you have to solve from the ones > you don't have to solve yet. > > Regards, > -Chap I solved it , but I need to use it in my new grammar, so I have to add in into core. That’s necessary. Thanks. But My own storage engine is implemented by extension. Extension is a good idea and I’m using it now.
Re: persist logical slots to disk during shutdown checkpoint
On Mon, Sep 11, 2023 at 02:49:49PM +0530, Amit Kapila wrote:
> I don't know if the difference is worth inventing a new BACKEND_TYPE_*
> but if you think so then we can probably discuss this in a new thread.
> I think we may want to improve some comments as a separate patch to
> make this evident.
The comments in postmaster.c could be improved, at least. There is no
need to discuss that here.
> This point is not very clear to me. Can you please quote the exact
> comment if you think something needs to be changed?
Hmm. Don't think that's it yet..
Please see the v11 attached, that rewords all the places of the patch
that need clarifications IMO. I've found that the comment additions
in CheckPointReplicationSlots() to be overcomplicated:
- The key point to force a flush of a slot if its confirmed_lsn has
moved ahead of the last LSN where it was saved is to make the follow
up restart more responsive.
- Not sure that there is any point to mention the other code paths in
the tree where ReplicationSlotSave() can be called, and a slot can be
saved in other processes than just WAL senders (like slot
manipulations in normal backends, for one). This was the last
sentence in v10.
- Persist is incorrect in this context in the tests, slot.c and
slot.h, as it should refer to the slot's data being flushed, saved or
just "made durable" because this is what the new last saved LSN is
here for. Persistence is a slot property, and does not refer to the
fact of flushing the data IMO.
+ if (s->data.invalidated == RS_INVAL_NONE &&
+ s->data.confirmed_flush != s->last_saved_confirmed_flush)
Actually this is incorrect, no? Shouldn't we make sure that the
confirmed_flush is strictly higher than the last saved LSN?
--
Michael
From 0aa4c8245359b60001ed90de1193dbb8b2b6c91c Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Fri, 14 Apr 2023 13:49:09 +0800
Subject: [PATCH v11] Flush logical slots to disk during a shutdown checkpoint
if required.
It's entirely possible for a logical slot to have a confirmed_flush LSN
higher than the last value saved on disk while not being marked as dirty.
Currently, it is not a major problem but a later patch adding support for
the upgrade of slots relies on that value being properly flushed to disk.
It can also help avoid processing the same transactions again in some
boundary cases after the clean shutdown and restart. Say, we process
some transactions for which we didn't send anything downstream (the
changes got filtered) but the confirm_flush LSN is updated due to
keepalives. As we don't flush the latest value of confirm_flush LSN, it
may lead to processing the same changes again without this patch.
The approach taken by this patch has been suggested by Ashutosh Bapat.
Author: Vignesh C, Julien Rouhaud, Kuroda Hayato
Reviewed-by: Amit Kapila, Dilip Kumar, Ashutosh Bapat, Peter Smith
Discussion: http://postgr.es/m/CAA4eK1JzJagMmb_E8D4au=gyqkxox0afnbm1fbp7sy7t4yw...@mail.gmail.com
Discussion: http://postgr.es/m/tyapr01mb58664c81887b3af2eb6b16e3f5...@tyapr01mb5866.jpnprd01.prod.outlook.com
---
src/include/replication/slot.h| 8 +-
src/backend/access/transam/xlog.c | 2 +-
src/backend/replication/slot.c| 32 +-
src/test/recovery/meson.build | 1 +
.../t/038_save_logical_slots_shutdown.pl | 102 ++
5 files changed, 140 insertions(+), 5 deletions(-)
create mode 100644 src/test/recovery/t/038_save_logical_slots_shutdown.pl
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index a8a89dc784..5e60030234 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -178,6 +178,12 @@ typedef struct ReplicationSlot
XLogRecPtr candidate_xmin_lsn;
XLogRecPtr candidate_restart_valid;
XLogRecPtr candidate_restart_lsn;
+
+ /*
+ * LSN used to track the last confirmed_flush LSN where the slot's data
+ * has been flushed to disk.
+ */
+ XLogRecPtr last_saved_confirmed_flush;
} ReplicationSlot;
#define SlotIsPhysical(slot) ((slot)->data.database == InvalidOid)
@@ -241,7 +247,7 @@ extern void ReplicationSlotNameForTablesync(Oid suboid, Oid relid, char *syncslo
extern void ReplicationSlotDropAtPubNode(WalReceiverConn *wrconn, char *slotname, bool missing_ok);
extern void StartupReplicationSlots(void);
-extern void CheckPointReplicationSlots(void);
+extern void CheckPointReplicationSlots(bool is_shutdown);
extern void CheckSlotRequirements(void);
extern void CheckSlotPermissions(void);
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f6f8adc72a..f26c8d18a6 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7039,7 +7039,7 @@ static void
CheckPointGuts(XLogRecPtr checkPointRedo, int flags)
{
CheckPointRelationMap();
- CheckPointReplicationSlots();
+ CheckPointReplicationSlots(flags & CHECKPOINT_IS_SHUTDOWN);
CheckPointSnapBuild();
Chec
Re: Make all Perl warnings fatal
On 10.08.23 07:58, Peter Eisentraut wrote: There are also a couple of issues in the MSVC legacy build system that would need to be tightened up in order to survive with fatal Perl warnings. Obviously, there is a question whether it's worth spending any time on that anymore. It looks like there are no principled objections to the overall idea here, but given this dependency on the MSVC build system removal, I'm going to set this patch to Returned with feedback for now.
Re: pg_rewind with cascade standby doesn't work well
On Mon, Sep 11, 2023 at 07:04:30PM +0300, Aleksander Alekseev wrote:
> Many thanks for submitting the patch. I added it to the nearest open
> commitfest [1].
>
> IMO a test is needed that makes sure no one is going to break this in
> the future.
You definitely need more complex test scenarios for that. If you can
come up with new ways to make the TAP tests of pg_rewind mode modular
in handling more complicated node setups, that would be a nice
addition, for example.
> [1]: https://commitfest.postgresql.org/45/4559/
@@ -7951,6 +7951,15 @@ xlog_redo(XLogReaderState *record)
ereport(PANIC,
(errmsg("unexpected timeline ID %u (should be %u) in
end-of-recovery record",
xlrec.ThisTimeLineID, replayTLI)));
+/*
+ * Update the control file so that crash recovery can follow the
timeline
+ * changes to this point.
+ */
+LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
+ControlFile->minRecoveryPoint = lsn;
+ControlFile->minRecoveryPointTLI = xlrec.ThisTimeLineID;
This patch is at least incorrect in its handling of crash recovery,
because these two should *never* be set in this case as we want to
replay up to the end of WAL. For example, see xlog.c or the top of
xlogrecovery.c about the assumptions behind these variables:
/* crash recovery should always recover to the end of WAL */
ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
ControlFile->minRecoveryPointTLI = 0;
If an end-of-recovery record is replayed during crash recovery, these
assumptions are plain broken.
One thing that we could consider is to be more aggressive with
restartpoints when replaying this record for a standby, see a few
lines above the lines added by your patch, for example. And we could
potentially emulate a post-promotion restart point to get a refresh of
the control file as it should, with the correct code paths involved in
the updates of minRecoveryPoint when the checkpointer does the job.
--
Michael
signature.asc
Description: PGP signature
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
On Wed, Sep 06, 2023 at 12:45:24AM +0900, Yugo NAGATA wrote: > I attached the update patch. I removed the incorrect comments and > unnecessary lines. Also, I rewrote the test to use "skip_all" instead > of SKIP because we skip the whole test rather than a part of it. Thanks for checking how IPC::Run behaves in this case on Windows! Right. This test is currently setting up a node for nothing, so let's skip this test entirely under $windows_os and move on. I'll backpatch that down to 15 once the embargo on REL_16_STABLE is lifted with the 16.0 tag. -- Michael signature.asc Description: PGP signature
Re: Row pattern recognition
>> What about leaving this (reevaluation) for now? Because: >> >> 1) we don't have CLASSIFIER >> 2) we don't allow to give CLASSIFIER to PREV as its arggument >> >> so I think we don't need to worry about this for now. > > Sure. I'm all for deferring features to make it easier to iterate; I > just want to make sure the architecture doesn't hit a dead end. Or at > least, not without being aware of it. Ok, let's defer this issue. Currently the patch already exceeds 3k lines. I am afraid too big patch cannot be reviewed by anyone, which means it will never be committed. > Also: is CLASSIFIER the only way to run into this issue? Good question. I would like to know. >> What if we don't follow the standard, instead we follow POSIX EREs? I >> think this is better for users unless RPR's REs has significant merit >> for users. > > Piggybacking off of what Vik wrote upthread, I think we would not be > doing ourselves any favors by introducing a non-compliant > implementation that performs worse than a traditional NFA. Those would > be some awful bug reports. What I am not sure about is, you and Vik mentioned that the traditional NFA is superior that POSIX NFA in terms of performance. But how "lexicographic ordering" is related to performance? >> I am not sure if we need to worry about this because of the reason I >> mentioned above. > > Even if we adopted POSIX NFA semantics, we'd still have to implement > our own parser for the PATTERN part of the query. I don't think > there's a good way for us to reuse the parser in src/backend/regex. Ok. >> > Does that seem like a workable approach? (Worst-case, my code is just >> > horrible, and we throw it in the trash.) >> >> Yes, it seems workable. I think for the first cut of RPR needs at >> least the +quantifier with reasonable performance. The current naive >> implementation seems to have issue because of exhaustive search. > > +1 BTW, attched is the v6 patch. The differences from v5 include: - Now aggregates can be used with RPR. Below is an example from the regression test cases, which is added by v6 patch. - Fix assersion error pointed out by Erik. SELECT company, tdate, price, first_value(price) OVER w, last_value(price) OVER w, max(price) OVER w, min(price) OVER w, sum(price) OVER w, avg(price) OVER w, count(price) OVER w FROM stock WINDOW w AS ( PARTITION BY company ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING AFTER MATCH SKIP PAST LAST ROW INITIAL PATTERN (START UP+ DOWN+) DEFINE START AS TRUE, UP AS price > PREV(price), DOWN AS price < PREV(price) ); company | tdate| price | first_value | last_value | max | min | sum | avg | count --++---+-++--+-+--+---+--- company1 | 07-01-2023 | 100 | 100 |140 | 200 | 100 | 590 | 147.5000 | 4 company1 | 07-02-2023 | 200 | || | | | | company1 | 07-03-2023 | 150 | || | | | | company1 | 07-04-2023 | 140 | || | | | | company1 | 07-05-2023 | 150 | || | | | | company1 | 07-06-2023 |90 | 90 |120 | 130 | 90 | 450 | 112.5000 | 4 company1 | 07-07-2023 | 110 | || | | | | company1 | 07-08-2023 | 130 | || | | | | company1 | 07-09-2023 | 120 | || | | | | company1 | 07-10-2023 | 130 | || | | | | company2 | 07-01-2023 |50 | 50 | 1400 | 2000 | 50 | 4950 | 1237.5000 | 4 company2 | 07-02-2023 | 2000 | || | | | | company2 | 07-03-2023 | 1500 | || | | | | company2 | 07-04-2023 | 1400 | || | | | | company2 | 07-05-2023 | 1500 | || | | | | company2 | 07-06-2023 |60 | 60 | 1200 | 1300 | 60 | 3660 | 915. | 4 company2 | 07-07-2023 | 1100 | || | | | | company2 | 07-08-2023 | 1300 | || | | | | company2 | 07-09-2023 | 1200 | || | | | | company2 | 07-10-2023 | 1300 | || | | |
Re: Make --help output fit within 80 columns per line
I like this work a lot. It's good to give developers easy to verify guidance about formatting the --help messages. However, I think instead of just adding a bunch of line breaks to satisfy the test, we should really try to make the lines shorter by rewording. Otherwise, chances are we are making the usability worse for many users, because it's more likely that part of the help will scroll off the screen. For example, in many cases, we could replace "do not" by "don't", or we could decrease the indentation of the second column by a few spaces, or just reword altogether. Also, it would be very useful if the TAP test function could print out the violating lines if a test fails. (Similar to how is() and like() print the failing values.) Maybe start with that, and then it's easier to play with different wording variants to try to make it fit better.
