Re: Missing free_var() at end of accum_sum_final()?
On 05.03.23 09:53, Joel Jacobson wrote: On Fri, Mar 3, 2023, at 16:11, Dean Rasheed wrote: Attachments: * make-result-using-vars-buf-v2.patch One suggestion: maybe add a comment explaining why the allocated buffer which size is based on strlen(cp) for the decimal digit values, is guaranteed to be large enough also for the result's digit buffer? I.e. some kind of proof why (NUMERIC_HDRSZ + strlen(cp) + DEC_DIGITS * 2) >= ((ndigits + 1) * sizeof(NumericDigit))) holds true in general. It looks like this thread has fizzled out, and no one is really working on the various proposed patch variants. Unless someone indicates that they are still seriously pursuing this, I will mark this patch as "Returned with feedback".
Re: pg_upgrade and logical replication
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.
>
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.
--
With Regards,
Amit Kapila.
Re: pg_upgrade and logical replication
On Mon, Sep 04, 2023 at 11:51:14AM +0530, Amit Kapila wrote: > +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. It is one of these patches that I have let aside for too long, and it solves a use-case of its own. I think that I could hack that pretty quickly given that Julien has done a bunch of the ground work. Would you agree with that? > Like the publisher-side upgrade patch [1], I think we should allow > upgrading subscriptions by default instead with some flag like > --preserve-subscription-state. If required, we can introduce --exclude > option for upgrade. Having it just for pg_dump sounds reasonable to > me. > > [1] - > https://www.postgresql.org/message-id/TYAPR01MB58664C81887B3AF2EB6B16E3F5939%40TYAPR01MB5866.jpnprd01.prod.outlook.com In the interface of the publisher for pg_upgrade agreed on and set in stone? I certainly agree to have a consistent upgrade experience for the two sides of logical replication, publications and subscriptions. Also, I'd rather have a filtering option at the same time as the upgrade option to give more control to users from the start. -- Michael signature.asc Description: PGP signature
Re: pg_upgrade and logical replication
On Thu, Apr 27, 2023 at 1:18 PM Hayato Kuroda (Fujitsu) wrote: > > 03. main > > Currently --preserve-subscription-state and --no-subscriptions can be used > together, but the situation is quite unnatural. Shouldn't we exclude them? > Right, that makes sense to me. -- With Regards, Amit Kapila.
Re: pg_upgrade and logical replication
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. Like the publisher-side upgrade patch [1], I think we should allow upgrading subscriptions by default instead with some flag like --preserve-subscription-state. If required, we can introduce --exclude option for upgrade. Having it just for pg_dump sounds reasonable to me. [1] - https://www.postgresql.org/message-id/TYAPR01MB58664C81887B3AF2EB6B16E3F5939%40TYAPR01MB5866.jpnprd01.prod.outlook.com -- With Regards, Amit Kapila.
Re: Exposing the lock manager's WaitForLockers() to SQL
I realized that for our use case, we'd ideally wait for holders of RowExclusiveLock only, and not e.g. VACUUM holding ShareUpdateExclusiveLock. Waiting for lockers in a specific mode seems possible by generalizing/duplicating WaitForLockersMultiple() and GetLockConflicts(), but I'd love to have a sanity check before attempting that. Also, I imagine those semantics might be too different to make sense as part of the LOCK command. Alternatively, I had originally been trying to use the pg_locks view, which obviously provides flexibility in identifying existing lock holders. But I couldn't find a way to wait for the locks to be released / transactions to finish, and I was a little concerned about the performance impact of selecting from it frequently when we only care about a subset of the locks, although I didn't try to assess that in our particular application. In any case, I'm looking forward to hearing more feedback from reviewers and potential users. :-)
Re: Impact of checkpointer during pg_upgrade
On Mon, Sep 4, 2023 at 10:33 AM Dilip Kumar wrote: > > On Mon, Sep 4, 2023 at 8:41 AM Amit Kapila wrote: > > > > On Sat, Sep 2, 2023 at 6:12 PM Dilip Kumar wrote: > > > > > > On Sat, Sep 2, 2023 at 10:09 AM Amit Kapila > > > wrote: > > > > > > > The other possibilities apart from not allowing an upgrade in such a > > > > case could be (a) Before starting the old cluster, we fetch the slots > > > > directly from the disk using some tool like [2] and make the decisions > > > > based on that state; > > > > > > Okay, so IIUC along with dumping the slot data we also need to dump > > > the latest checkpoint LSN because during upgrade we do check that the > > > confirmed flush lsn for all the slots should be the same as the latest > > > checkpoint. Yeah but I think we could work this out. > > > > > We already have the latest checkpoint LSN information from > > pg_controldata. I think we can use that as the patch proposed in the > > thread [1] is doing now. Do you have something else in mind? > > I think I did not understood the complete proposal. And what I meant > is that if we dump the slot before we start the cluster thats fine. > But then later after starting the old cluster if we run some query > like we do in check_old_cluster_for_valid_slots() then thats not > right, because there is gap between the status of the slots what we > dumped before starting the cluster and what we are checking after the > cluster, so there is not point of that check right? > That's right but if we do read slots from disk, we preserve those in the memory and use that information instead of querying it again in check_old_cluster_for_valid_slots(). > > > (b) During the upgrade, we don't allow WAL to be > > > > removed if it can invalidate slots; (c) Copy/Migrate the invalid slots > > > > as well but for that, we need to expose an API to invalidate the > > > > slots; > > > > > > (d) somehow distinguish the slots that are invalidated during > > > > an upgrade and then simply copy such slots because anyway we ensure > > > > that all the WAL required by slot is sent before shutdown. > > > > > > Yeah this could also be an option, although we need to think the > > > mechanism of distinguishing those slots looks clean and fit well with > > > other architecture. > > > > > > > If we want to do this we probably need to maintain a flag in the slot > > indicating that it was invalidated during an upgrade and then use the > > same flag in the upgrade to check the validity of slots. I think such > > a flag needs to be maintained at the same level as > > ReplicationSlotInvalidationCause to avoid any inconsistency among > > those. > > I think we can do better, like we can just read the latest > checkpoint's LSN before starting the old cluster. And now while > checking the slot can't we check if the the slot is invalidated then > their confirmed_flush_lsn >= the latest_checkpoint_lsn we preserved > before starting the cluster because if so then those slot might have > got invalidated during the upgrade no? > Isn't that possible only if we update confirmend_flush LSN while invalidating? Otherwise, how the check you are proposing can succeed? > > > > > Alternatively can't we just ignore all the invalidated slots and do > > > not migrate them at all. Because such scenarios are very rare that > > > some of the segments are getting dropped just during the upgrade time > > > and that too from the old cluster so in such cases not migrating the > > > slots which are invalidated should be fine no? > > > > > > > I also think that such a scenario would be very rare but are you > > suggesting to ignore all invalidated slots or just the slots that got > > invalidated during an upgrade? BTW, if we simply ignore invalidated > > slots then users won't be able to drop corresponding subscriptions > > after an upgrade. They need to first use the Alter Subscription > > command to disassociate the slot (by using the command ALTER > > SUBSCRIPTION ... SET (slot_name = NONE)) and then drop the > > subscription similar to what we suggest in other cases as described in > > the Replication Slot Management section in docs [2]. > > Yeah I think thats not the best thing to do. > Right, but OTOH, there is an argument to just document it instead of having additional complexities in the code as such cases should be rare. -- With Regards, Amit Kapila.
Optimize planner memory consumption for huge arrays
Hi, hackers, Looking at the planner behaviour with the memory consumption patch [1], I figured out that arrays increase memory consumption by the optimizer significantly. See init.sql in attachment. The point here is that the planner does small memory allocations for each element during estimation. As a result, it looks like the planner consumes about 250 bytes for each integer element. It is maybe not a problem most of the time. However, in the case of partitions, memory consumption multiplies by each partition. Such a corner case looks weird, but the fix is simple. So, why not? The diff in the attachment is proof of concept showing how to reduce wasting of memory. Having benchmarked a bit, I didn't find any overhead. [1] Report planning memory in EXPLAIN ANALYZE https://www.postgresql.org/message-id/flat/CAExHW5sZA=5lj_zppro-w09ck8z9p7eayaqq3ks9gdfhrxe...@mail.gmail.com -- Regards, Andrey Lepikhov init.sql Description: application/sql array_compact_memusage.diff Description: Binary data
Re: Impact of checkpointer during pg_upgrade
On Mon, Sep 4, 2023 at 8:41 AM Amit Kapila wrote: > > On Sat, Sep 2, 2023 at 6:12 PM Dilip Kumar wrote: > > > > On Sat, Sep 2, 2023 at 10:09 AM Amit Kapila wrote: > > > > > The other possibilities apart from not allowing an upgrade in such a > > > case could be (a) Before starting the old cluster, we fetch the slots > > > directly from the disk using some tool like [2] and make the decisions > > > based on that state; > > > > Okay, so IIUC along with dumping the slot data we also need to dump > > the latest checkpoint LSN because during upgrade we do check that the > > confirmed flush lsn for all the slots should be the same as the latest > > checkpoint. Yeah but I think we could work this out. > > > We already have the latest checkpoint LSN information from > pg_controldata. I think we can use that as the patch proposed in the > thread [1] is doing now. Do you have something else in mind? I think I did not understood the complete proposal. And what I meant is that if we dump the slot before we start the cluster thats fine. But then later after starting the old cluster if we run some query like we do in check_old_cluster_for_valid_slots() then thats not right, because there is gap between the status of the slots what we dumped before starting the cluster and what we are checking after the cluster, so there is not point of that check right? > > (b) During the upgrade, we don't allow WAL to be > > > removed if it can invalidate slots; (c) Copy/Migrate the invalid slots > > > as well but for that, we need to expose an API to invalidate the > > > slots; > > > > (d) somehow distinguish the slots that are invalidated during > > > an upgrade and then simply copy such slots because anyway we ensure > > > that all the WAL required by slot is sent before shutdown. > > > > Yeah this could also be an option, although we need to think the > > mechanism of distinguishing those slots looks clean and fit well with > > other architecture. > > > > If we want to do this we probably need to maintain a flag in the slot > indicating that it was invalidated during an upgrade and then use the > same flag in the upgrade to check the validity of slots. I think such > a flag needs to be maintained at the same level as > ReplicationSlotInvalidationCause to avoid any inconsistency among > those. I think we can do better, like we can just read the latest checkpoint's LSN before starting the old cluster. And now while checking the slot can't we check if the the slot is invalidated then their confirmed_flush_lsn >= the latest_checkpoint_lsn we preserved before starting the cluster because if so then those slot might have got invalidated during the upgrade no? > > > Alternatively can't we just ignore all the invalidated slots and do > > not migrate them at all. Because such scenarios are very rare that > > some of the segments are getting dropped just during the upgrade time > > and that too from the old cluster so in such cases not migrating the > > slots which are invalidated should be fine no? > > > > I also think that such a scenario would be very rare but are you > suggesting to ignore all invalidated slots or just the slots that got > invalidated during an upgrade? BTW, if we simply ignore invalidated > slots then users won't be able to drop corresponding subscriptions > after an upgrade. They need to first use the Alter Subscription > command to disassociate the slot (by using the command ALTER > SUBSCRIPTION ... SET (slot_name = NONE)) and then drop the > subscription similar to what we suggest in other cases as described in > the Replication Slot Management section in docs [2]. Yeah I think thats not the best thing to do. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: [PoC] pg_upgrade: allow to upgrade publisher node
On Fri, Sep 1, 2023 at 6:34 PM Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Dilip,
>
> Thank you for reviewing!
>
> >
> > 1.
> > + conn = connectToServer(&new_cluster, "template1");
> > +
> > + prep_status("Checking for logical replication slots");
> > +
> > + res = executeQueryOrDie(conn, "SELECT slot_name "
> > + "FROM pg_catalog.pg_replication_slots "
> > + "WHERE slot_type = 'logical' AND "
> > + "temporary IS FALSE;");
> >
> >
> > I think we should add some comment saying this query will only fetch
> > logical slots because the database name will always be NULL in the
> > physical slots. Otherwise looking at the query it is very confusing
> > how it is avoiding the physical slots.
>
> Hmm, the query you pointed out does not check the database of the slot...
> We are fetching only logical slots by the condition "slot_type = 'logical'",
> I think it is too trivial to describe in the comment.
> Just to confirm - pg_replication_slots can see alls the slots even if the
> database
> is not current one.
I think this is fine. Actually I posted comments based on v28 where
the query inside get_old_cluster_logical_slot_infos_per_db() function
was missing the condition on the slot_type = logical but while
commenting I quoted the wrong hunk from the code. Anyway the other
part of the code which I intended is also fixed from v29 so all good.
Thanks :)
> > 4. Looking at the above two comments I feel that now the order should be
> > like
> > - Fetch all the db infos
> > get_db_infos()
> > - loop
> >get_rel_infos()
> >get_old_cluster_logical_slot_infos()
> >
> > -- and now print relation and slot information per database
> > print_db_infos()
>
> Fixed like that. It seems that we go back to old style...
> Now the debug prints are like below:
>
> ```
> source databases:
> Database: "template1"
> relname: "pg_catalog.pg_largeobject", reloid: 2613, reltblspace: ""
> relname: "pg_catalog.pg_largeobject_loid_pn_index", reloid: 2683,
> reltblspace: ""
> Database: "postgres"
> relname: "pg_catalog.pg_largeobject", reloid: 2613, reltblspace: ""
> relname: "pg_catalog.pg_largeobject_loid_pn_index", reloid: 2683,
> reltblspace: ""
> Logical replication slots within the database:
> slotname: "old1", plugin: "test_decoding", two_phase: 0
> slotname: "old2", plugin: "test_decoding", two_phase: 0
> slotname: "old3", plugin: "test_decoding", two_phase: 0
Yeah this looks good now.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
REL_15_STABLE: pgbench tests randomly failing on CI, Windows only
Hi, Somehow these tests have recently become unstable and have failed a few times: https://github.com/postgres/postgres/commits/REL_15_STABLE The failures are like: [22:32:26.722] # Failed test 'pgbench simple update stdout /(?^:builtin: simple update)/' [22:32:26.722] # at t/001_pgbench_with_server.pl line 119. [22:32:26.722] # 'pgbench (15.4) [22:32:26.722] # ' [22:32:26.722] # doesn't match '(?^:builtin: simple update)'
Re: Impact of checkpointer during pg_upgrade
On Sat, Sep 2, 2023 at 6:12 PM Dilip Kumar wrote: > > On Sat, Sep 2, 2023 at 10:09 AM Amit Kapila wrote: > > > The other possibilities apart from not allowing an upgrade in such a > > case could be (a) Before starting the old cluster, we fetch the slots > > directly from the disk using some tool like [2] and make the decisions > > based on that state; > > Okay, so IIUC along with dumping the slot data we also need to dump > the latest checkpoint LSN because during upgrade we do check that the > confirmed flush lsn for all the slots should be the same as the latest > checkpoint. Yeah but I think we could work this out. > We already have the latest checkpoint LSN information from pg_controldata. I think we can use that as the patch proposed in the thread [1] is doing now. Do you have something else in mind? > (b) During the upgrade, we don't allow WAL to be > > removed if it can invalidate slots; (c) Copy/Migrate the invalid slots > > as well but for that, we need to expose an API to invalidate the > > slots; > > (d) somehow distinguish the slots that are invalidated during > > an upgrade and then simply copy such slots because anyway we ensure > > that all the WAL required by slot is sent before shutdown. > > Yeah this could also be an option, although we need to think the > mechanism of distinguishing those slots looks clean and fit well with > other architecture. > If we want to do this we probably need to maintain a flag in the slot indicating that it was invalidated during an upgrade and then use the same flag in the upgrade to check the validity of slots. I think such a flag needs to be maintained at the same level as ReplicationSlotInvalidationCause to avoid any inconsistency among those. > Alternatively can't we just ignore all the invalidated slots and do > not migrate them at all. Because such scenarios are very rare that > some of the segments are getting dropped just during the upgrade time > and that too from the old cluster so in such cases not migrating the > slots which are invalidated should be fine no? > I also think that such a scenario would be very rare but are you suggesting to ignore all invalidated slots or just the slots that got invalidated during an upgrade? BTW, if we simply ignore invalidated slots then users won't be able to drop corresponding subscriptions after an upgrade. They need to first use the Alter Subscription command to disassociate the slot (by using the command ALTER SUBSCRIPTION ... SET (slot_name = NONE)) and then drop the subscription similar to what we suggest in other cases as described in the Replication Slot Management section in docs [2]. Also, if users really want to continue that subscription by syncing corresponding tables then they can recreate the slots manually and then continue with replication. So, if we want to do this then we will just rely on the current state (at the time we query for them in the old cluster) of slots, and even if they later got invalidated during the upgrade, we will just ignore such invalidations as anyway the required WAL is already copied. [1] - https://www.postgresql.org/message-id/TYAPR01MB58664C81887B3AF2EB6B16E3F5939%40TYAPR01MB5866.jpnprd01.prod.outlook.com [2] - https://www.postgresql.org/docs/devel/logical-replication-subscription.html -- With Regards, Amit Kapila.
Re: Assert failure in ATPrepAddPrimaryKey
On Fri, Sep 1, 2023 at 7:48 PM Alvaro Herrera wrote: > On 2023-Sep-01, Richard Guo wrote: > > > I ran into an Assert failure in ATPrepAddPrimaryKey() with the query > > below: > > > > CREATE TABLE t0(c0 boolean); > > CREATE TABLE t1() INHERITS(t0); > > > > # ALTER TABLE t0 ADD CONSTRAINT m EXCLUDE ((1) WITH =); > > server closed the connection unexpectedly > > Ugh, right, I failed to make the new function do nothing for this case; > this had no coverage. Fix attached, with some additional test cases > based on yours. Thanks for the fix! Thanks Richard
Re: Avoid a possible null pointer (src/backend/utils/adt/pg_locale.c)
On Sat, Sep 02, 2023 at 09:13:11AM -0300, Ranier Vilela wrote: > I tried to keep the same behavior as before. > Note that if the locale equals COLLPROVIDER_LIBC, > the message to the user will be the same. -/* shouldn't happen */ -elog(ERROR, "unsupported collprovider: %c", locale->provider); +elog(ERROR, "collprovider '%c' does not support pg_strnxfrm_prefix()", + locale->provider); Based on what's written here, these messages could be better because full sentences are not encouraged in error messages, even for non-translated elogs: https://www.postgresql.org/docs/current/error-style-guide.html Perhaps something like "could not use collprovider %c: no support for %s", where the function name is used, would be more consistent. -- Michael signature.asc Description: PGP signature
Re: Cleaning up array_in()
hi.
attached v4.
v4, 0001 to 0005 is the same as v3 in
https://www.postgresql.org/message-id/5859ce4e-2be4-92b0-c85c-e1e24eab57c6%40iki.fi
v4-0006 doing some modifications to address the corner case mentioned
in the previous thread (like select '{{1,},{1},}'::text[]).
also fixed all these FIXME, Heikki mentioned in the code.
v4-0007 refactor ReadDimensionInt. to make the array dimension bound
variables within the INT_MIN and INT_MAX. so it will make select
'[21474836488:21474836489]={1,2}'::int[]; fail.
From 1a8251c6445bc37bbae156d70b4cbd3c93a9d29d Mon Sep 17 00:00:00 2001
From: pgaddict
Date: Sat, 2 Sep 2023 17:59:59 +0800
Subject: [PATCH v4 6/7] refactor to make corner case '{1,}'::int[] fail. that
is before right curly brace, we should not precede it with a comma seperator.
---
src/backend/utils/adt/arrayfuncs.c | 48 +---
src/test/regress/expected/arrays.out | 36 -
src/test/regress/sql/arrays.sql | 8 +
3 files changed, 73 insertions(+), 19 deletions(-)
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 19d7af6d..cce6b564 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -233,6 +233,11 @@ array_in(PG_FUNCTION_ARGS)
typdelim = my_extra->typdelim;
typioparam = my_extra->typioparam;
+ for (int i = 0; i < MAXDIM; i++)
+ {
+ dim[i] = -1;
+ lBound[i] = 1;
+ }
/*
* Start processing the input string.
*
@@ -245,14 +250,12 @@ array_in(PG_FUNCTION_ARGS)
if (ndim == 0)
{
- /* No array dimensions, so intuit dimensions from brace structure */
+ /* No array dimensions, so init dimensions from brace structure */
if (*p != '{')
ereturn(escontext, (Datum) 0,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("malformed array literal: \"%s\"", string),
errdetail("Array value must start with \"{\" or dimension information.")));
- for (int i = 0; i < MAXDIM; i++)
- lBound[i] = 1;
}
else
{
@@ -304,28 +307,17 @@ array_in(PG_FUNCTION_ARGS)
initStringInfo(&buf);
appendStringInfo(&buf, "array_in- ndim %d (", ndim);
- for (int i = 0; i < ndim; i++)
+ for (int i = 0; i < MAXDIM; i++)
appendStringInfo(&buf, " %d", dim[i]);
+ appendStringInfo(&buf, "lBound info");
+ for (int i = 0; i < MAXDIM; i++)
+ appendStringInfo(&buf, " %d", lBound[i]);
appendStringInfo(&buf, ") for %s\n", string);
elog(NOTICE, "%s", buf.data);
pfree(buf.data);
}
#endif
- /* This checks for overflow of the array dimensions */
-
- /*
- * FIXME: Is this still required? I believe all the checks it performs are
- * redundant with other checks in ReadArrayDimension() and ReadArrayStr()
- */
- nitems_according_to_dims = ArrayGetNItemsSafe(ndim, dim, escontext);
- if (nitems_according_to_dims < 0)
- PG_RETURN_NULL();
- if (nitems != nitems_according_to_dims)
- elog(ERROR, "mismatch nitems, %d vs %d", nitems, nitems_according_to_dims);
- if (!ArrayCheckBoundsSafe(ndim, dim, lBound, escontext))
- PG_RETURN_NULL();
-
/* Empty array? */
if (nitems == 0)
PG_RETURN_ARRAYTYPE_P(construct_empty_array(element_type));
@@ -675,10 +667,30 @@ ReadArrayStr(char **srcptr,
{
/* Nested sub-arrays count as elements of outer level */
nelems[nest_level - 1]++;
+
+ if (nitems > 0 && expect_delim == false)
+ {
+ ereturn(escontext, false,
+(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("malformed array literal: \"%s\"", origStr),
+ errdetail("Unexpected \"%c\" character.",
+ typdelim)));
+ }
expect_delim = true;
}
else
+{
+ /* rightmost should precede with element or bracket */
+ if (nitems > 0 && expect_delim == false)
+ {
+ ereturn(escontext, false,
+(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+errmsg("malformed array literal: \"%s\"", origStr),
+errdetail("Unexpected \"%c\" character.",
+ typdelim)));
+ }
expect_delim = false;
+}
if (dim[nest_level] < 0)
{
diff --git a/src/test/regress/expected/arrays.out b/src/test/regress/expected/arrays.out
index 93e11068..122f073e 100644
--- a/src/test/regress/expected/arrays.out
+++ b/src/test/regress/expected/arrays.out
@@ -1520,7 +1520,7 @@ select '{{1},{}}'::text[];
ERROR: malformed array literal: "{{1},{}}"
LINE 1: select '{{1},{}}'::text[];
^
-DETAIL: Multidimensional arrays must have sub-arrays with matching dimensions.
+DETAIL: Unexpected "," character.
select '{{},{1}}'::text[];
ERROR: malformed array literal: "{{},{1}}"
LINE 1: select '{{},{1}}'::text[];
@@ -1544,6 +1544,34 @@ ERROR: cannot determine type of empty array
LINE 1: select array[];
^
HINT: Explicitly cast to the desired type, for example ARRAY[]::integer[].
+select '{{1,},{1},}'::text[];
+ERROR: malformed array literal: "{{1,},{1},}"
+LINE 1: select '{{1,},{1},}'::text[];
+ ^
+DETAIL: Unexpected "
Re: Why doesn't Vacuum FULL update the VM
On Fri, Sep 1, 2023 at 8:38 PM Peter Geoghegan wrote:
>
> On Fri, Sep 1, 2023 at 12:34 PM Melanie Plageman
> wrote:
> > I don't see why the visibility map shouldn't be updated so that all of
> > the pages show all visible and all frozen for this relation after the
> > vacuum full.
>
> There was a similar issue with COPY FREEZE. It was fixed relatively
> recently -- see commit 7db0cd21.
Thanks for digging that up for me!
My first thought after looking a bit at the vacuum full/cluster code
is that we could add an all_visible flag to the RewriteState and set
it to false in heapam_relation_copy_for_cluster() in roughly the same
cases as heap_page_is_all_visible(), then, if rwstate->all_visible is
true in raw_heap_insert(), when we need to advance to the next block,
we set the page all visible and update the VM. Either way, we reset
all_visible to true since we are advancing to the next block.
I wrote a rough outline of that idea in the attached patches. It
doesn't emit WAL for the VM update or handle toast tables or anything
(it is just a rough sketch), but I just wondered if this was in the
right direction.
- Melanie
From eda40534e169e0bac0d11d89947130b44653b925 Mon Sep 17 00:00:00 2001
From: Melanie Plageman
Date: Sun, 3 Sep 2023 16:17:28 -0400
Subject: [PATCH v0 3/3] set all_visible in VM vac full
---
src/backend/access/heap/heapam_handler.c | 15 ++-
src/backend/access/heap/rewriteheap.c| 16
2 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 5a17112c91..a37be8cfef 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -791,6 +791,7 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap,
HeapTuple tuple;
Buffer buf;
bool isdead;
+ TransactionId xmin;
CHECK_FOR_INTERRUPTS();
@@ -853,10 +854,18 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap,
break;
case HEAPTUPLE_RECENTLY_DEAD:
*tups_recently_dead += 1;
-/* fall through */
+isdead = false;
+rwstate->all_visible = false;
+break;
case HEAPTUPLE_LIVE:
/* Live or recently dead, must copy it */
+xmin = HeapTupleHeaderGetXmin(tuple->t_data);
isdead = false;
+if (!HeapTupleHeaderXminCommitted(tuple->t_data))
+ rwstate->all_visible = false;
+
+else if (!TransactionIdPrecedes(xmin, OldestXmin))
+ rwstate->all_visible = false;
break;
case HEAPTUPLE_INSERT_IN_PROGRESS:
@@ -873,6 +882,7 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap,
elog(WARNING, "concurrent insert in progress within table \"%s\"",
RelationGetRelationName(OldHeap));
/* treat as live */
+rwstate->all_visible = false;
isdead = false;
break;
case HEAPTUPLE_DELETE_IN_PROGRESS:
@@ -886,6 +896,7 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap,
RelationGetRelationName(OldHeap));
/* treat as recently dead */
*tups_recently_dead += 1;
+rwstate->all_visible = false;
isdead = false;
break;
default:
@@ -906,6 +917,8 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap,
*tups_vacuumed += 1;
*tups_recently_dead -= 1;
}
+ else
+rwstate->all_visible = false;
continue;
}
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 93e75dc7c2..b3d63200d4 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -111,6 +111,7 @@
#include "access/transam.h"
#include "access/xact.h"
#include "access/xloginsert.h"
+#include "access/visibilitymap.h"
#include "catalog/catalog.h"
#include "common/file_utils.h"
#include "lib/ilist.h"
@@ -234,6 +235,7 @@ begin_heap_rewrite(Relation old_heap, Relation new_heap, TransactionId oldest_xm
state->rs_freeze_xid = freeze_xid;
state->rs_cutoff_multi = cutoff_multi;
state->rs_cxt = rw_cxt;
+ state->all_visible = true;
/* Initialize hash tables used to track update chains */
hash_ctl.keysize = sizeof(TidHashKey);
@@ -647,6 +649,8 @@ raw_heap_insert(RewriteState state, HeapTuple tup)
* enforce saveFreeSpace unconditionally.
*/
+ if (state->all_visible)
+PageSetAllVisible(page);
/* XLOG stuff */
if (RelationNeedsWAL(state->rs_new_rel))
log_newpage(&state->rs_new_rel->rd_locator,
@@ -655,6 +659,18 @@ raw_heap_insert(RewriteState state, HeapTuple tup)
page,
true);
+ if (state->all_visible)
+ {
+Buffer vmbuffer = InvalidBuffer;
+visibilitymap_pin(state->rs_new_rel, state->rs_blockno, &vmbuffer);
+visibilitymap_set_unbuffered(state->rs_new_rel, state->rs_blockno, vmbuffer,
+ VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN);
+if (BufferIsValid(vmbuffer))
+ ReleaseBuffer(vmbuffer);
+ }
+
+ state->all_visible = true;
+
/*
*
Re: proposal: psql: show current user in prompt
Hi
ne 3. 9. 2023 v 9:59 odesílatel Jelte Fennema napsal:
> On Sun, 3 Sept 2023 at 08:24, Pavel Stehule
> wrote:
> > My personal feeling from this area is that the protocol design is done,
> but it is not implemented on libpq level. My feelings can be wrong. The
> protocol number is hardcoded in libpq, so I cannot change it from the
> client side.
>
>
> No, I agree you're right the client side code to fall back to older
> versions is not implemented. But that seems fairly simple to do. We
> can change pqGetNegotiateProtocolVersion3 its behaviour. That function
> should change conn->pversion to the server provided version if it's
> lower than the client version (as long as the server provided version
> is 3.0 or larger). And then we should return an error when calling
> PQlinkParameterStatus/PQunlinkParameterStatus if the pversion is not
> high enough.
>
ok
here is an try
Regards
Pavel
From 80a076e28c293aef96f455a25e967f2c9baf8eac Mon Sep 17 00:00:00 2001
From: "ok...@github.com"
Date: Sun, 3 Sep 2023 19:14:24 +0200
Subject: [PATCH 3/4] - allow to connect to server with major protocol version
3, minor version is ignored
- allow to read minor protocol version
---
src/include/libpq/pqcomm.h | 2 +-
src/interfaces/libpq/exports.txt| 1 +
src/interfaces/libpq/fe-connect.c | 12 +++-
src/interfaces/libpq/fe-protocol3.c | 13 ++---
src/interfaces/libpq/libpq-fe.h | 1 +
5 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h
index 46a0946b8b..4ea4538157 100644
--- a/src/include/libpq/pqcomm.h
+++ b/src/include/libpq/pqcomm.h
@@ -94,7 +94,7 @@ is_unixsock_path(const char *path)
*/
#define PG_PROTOCOL_EARLIEST PG_PROTOCOL(3,0)
-#define PG_PROTOCOL_LATEST PG_PROTOCOL(3,0)
+#define PG_PROTOCOL_LATEST PG_PROTOCOL(3,1)
typedef uint32 ProtocolVersion; /* FE/BE protocol version number */
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 7e101368d5..71e180cd57 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -193,3 +193,4 @@ PQsendClosePrepared 190
PQsendClosePortal 191
PQlinkParameterStatus 192
PQunlinkParameterStatus 193
+PQprotocolSubversion 194
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index bf83a9b569..5e628578b4 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2777,7 +2777,7 @@ keep_going: /* We will come back to here until there is
* must persist across individual connection attempts, but we must
* reset them when we start to consider a new server.
*/
- conn->pversion = PG_PROTOCOL(3, 0);
+ conn->pversion = PG_PROTOCOL(3, 1);
conn->send_appname = true;
#ifdef USE_SSL
/* initialize these values based on SSL mode */
@@ -7234,6 +7234,16 @@ PQprotocolVersion(const PGconn *conn)
return PG_PROTOCOL_MAJOR(conn->pversion);
}
+int
+PQprotocolSubversion(const PGconn *conn)
+{
+ if (!conn)
+ return 0;
+ if (conn->status == CONNECTION_BAD)
+ return 0;
+ return PG_PROTOCOL_MINOR(conn->pversion);
+}
+
int
PQserverVersion(const PGconn *conn)
{
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index 90d4e17e6f..05afa70694 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -1441,9 +1441,16 @@ pqGetNegotiateProtocolVersion3(PGconn *conn)
}
if (their_version < conn->pversion)
- libpq_append_conn_error(conn, "protocol version not supported by server: client uses %u.%u, server supports up to %u.%u",
-PG_PROTOCOL_MAJOR(conn->pversion), PG_PROTOCOL_MINOR(conn->pversion),
-PG_PROTOCOL_MAJOR(their_version), PG_PROTOCOL_MINOR(their_version));
+ {
+ /* The protocol 3.0 is required */
+ if (PG_PROTOCOL_MAJOR(their_version) == 3)
+ conn->pversion = their_version;
+ else
+ libpq_append_conn_error(conn, "protocol version not supported by server: client uses %u.%u, server supports up to %u.%u",
+ PG_PROTOCOL_MAJOR(conn->pversion), PG_PROTOCOL_MINOR(conn->pversion),
+ PG_PROTOCOL_MAJOR(their_version), PG_PROTOCOL_MINOR(their_version));
+ }
+
if (num > 0)
{
appendPQExpBuffer(&conn->errorMessage,
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index ba3ad7e0aa..904e611670 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -347,6 +347,7 @@ extern PGTransactionStatusType PQtransactionStatus(const PGconn *conn);
extern const char *PQparameterStatus(const PGconn *conn,
const char *paramName);
extern int PQprotocolVersion(const PGconn *conn);
+extern int PQprotocolSubversion(const PGconn *conn);
extern int PQserverVersion(const PGconn *conn);
extern char *PQerrorMessage(const PGconn *conn);
extern int PQsocket(const PGconn *conn);
--
2.41.0
From bd59c5318c09d09b764da81533d2712aa6d658d4 Mon Sep 17 00:
Re: Inefficiency in parallel pg_restore with many tables
On Sun, Sep 03, 2023 at 12:04:00PM +0200, Alvaro Herrera wrote: > On 2023-Sep-02, Nathan Bossart wrote: >> I ended up hacking together a (nowhere near committable) patch to see how >> hard it would be to allow using any type with binaryheap. It doesn't seem >> too bad. > > Yeah, using void * seems to lead to interfaces that are pretty much the > same as bsearch() or qsort(). Right. This is what I had in mind. > (Why isn't your payload type const, > though?) It probably should be const. This patch was just a proof-of-concept and still requireѕ a bit of work. > I do wonder why did you change _remove_first and _first to have a > 'result' output argument instead of a return value. Does this change > actually buy you anything? simplehash.h doesn't do that either. > >> -extern void binaryheap_add(binaryheap *heap, Datum d); >> -extern Datum binaryheap_first(binaryheap *heap); >> -extern Datum binaryheap_remove_first(binaryheap *heap); >> -extern void binaryheap_replace_first(binaryheap *heap, Datum d); >> +extern void binaryheap_add(binaryheap *heap, void *d); >> +extern void binaryheap_first(binaryheap *heap, void *result); >> +extern void binaryheap_remove_first(binaryheap *heap, void *result); >> +extern void binaryheap_replace_first(binaryheap *heap, void *d); _first could likely just return a pointer to the data in the binary heap's array. However, _remove_first has to copy the data somewhere, so I think the alternative would be to return a palloc'd value. Is there another way that I'm not thinking of? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: Buildfarm failures on urocryon
On Fri, 1 Sept 2023 at 20:20, Mark Wong wrote: > > Hi, > > On Fri, Sep 01, 2023 at 11:27:47AM +0530, vignesh C wrote: > > Hi, > > > > Recently urocryon has been failing with the following errors at [1]: > > checking for icu-uc icu-i18n... no > > configure: error: ICU library not found > > If you have ICU already installed, see config.log for details on the > > failure. It is possible the compiler isn't looking in the proper directory. > > Use --without-icu to disable ICU support. > > > > configure:8341: checking whether to build with ICU support > > configure:8371: result: yes > > configure:8378: checking for icu-uc icu-i18n > > configure:8440: result: no > > configure:8442: error: ICU library not found > > If you have ICU already installed, see config.log for details on the > > failure. It is possible the compiler isn't looking in the proper directory. > > Use --without-icu to disable ICU support. > > > > Urocryon has been failing for the last 17 days. > > > > I think ICU libraries need to be installed in urocryon to fix this issue. > > Oops, that's when I upgraded the build farm client (from v14 to v17). I > think it's fixed now... Thanks, this issue is fixed now. Regards, Vignesh
Re: Inefficiency in parallel pg_restore with many tables
On 2023-Sep-02, Nathan Bossart wrote: > On Fri, Sep 01, 2023 at 01:52:48PM -0700, Nathan Bossart wrote: > > Yeah, something similar to simplehash for binary heaps could be nice. That > > being said, I don't know if there's a strong reason to specialize the > > implementation for a given C data type in most cases. > > I ended up hacking together a (nowhere near committable) patch to see how > hard it would be to allow using any type with binaryheap. It doesn't seem > too bad. Yeah, using void * seems to lead to interfaces that are pretty much the same as bsearch() or qsort(). (Why isn't your payload type const, though?) I do wonder why did you change _remove_first and _first to have a 'result' output argument instead of a return value. Does this change actually buy you anything? simplehash.h doesn't do that either. > -extern void binaryheap_add(binaryheap *heap, Datum d); > -extern Datum binaryheap_first(binaryheap *heap); > -extern Datum binaryheap_remove_first(binaryheap *heap); > -extern void binaryheap_replace_first(binaryheap *heap, Datum d); > +extern void binaryheap_add(binaryheap *heap, void *d); > +extern void binaryheap_first(binaryheap *heap, void *result); > +extern void binaryheap_remove_first(binaryheap *heap, void *result); > +extern void binaryheap_replace_first(binaryheap *heap, void *d); -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: proposal: psql: show current user in prompt
On Sun, 3 Sept 2023 at 08:24, Pavel Stehule wrote: > My personal feeling from this area is that the protocol design is done, but > it is not implemented on libpq level. My feelings can be wrong. The protocol > number is hardcoded in libpq, so I cannot change it from the client side. No, I agree you're right the client side code to fall back to older versions is not implemented. But that seems fairly simple to do. We can change pqGetNegotiateProtocolVersion3 its behaviour. That function should change conn->pversion to the server provided version if it's lower than the client version (as long as the server provided version is 3.0 or larger). And then we should return an error when calling PQlinkParameterStatus/PQunlinkParameterStatus if the pversion is not high enough.
