Re: GetNewObjectId question
On Sat, Dec 10, 2022 at 11:03:38PM -0800, Maciek Sakrejda wrote: > Sure. My C is pretty limited, but I think it's just the attached? I > patterned the usage on the way this is done in CreateRole. It passes > check-world here. Looks OK seen from here. Thanks for the patch! I don't have much freshness and time today, but I can get back to this thread tomorrow. -- Michael signature.asc Description: PGP signature
Re: GetNewObjectId question
Sure. My C is pretty limited, but I think it's just the attached? I patterned the usage on the way this is done in CreateRole. It passes check-world here. From c7cae5d3e8d179505f26851f1241436a3748f9a8 Mon Sep 17 00:00:00 2001 From: Maciek Sakrejda Date: Sat, 10 Dec 2022 22:51:02 -0800 Subject: [PATCH] Replace GetNewObjectId call with GetNewOidWithIndex GetNewObjectId is not intended to be used directly in most situations, since it can lead to duplicate OIDs unless that is guarded against. --- src/backend/commands/user.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c index 8b6543edee..0686807fa0 100644 --- a/src/backend/commands/user.c +++ b/src/backend/commands/user.c @@ -1850,7 +1850,8 @@ AddRoleMems(const char *rolename, Oid roleid, } /* get an OID for the new row and insert it */ - objectId = GetNewObjectId(); + objectId = GetNewOidWithIndex(pg_authmem_rel, AuthMemOidIndexId, + Anum_pg_auth_members_oid); new_record[Anum_pg_auth_members_oid - 1] = objectId; tuple = heap_form_tuple(pg_authmem_dsc, new_record, new_record_nulls); -- 2.25.1
Re: Progress report of CREATE INDEX for nested partitioned tables
On Sat, Dec 10, 2022 at 12:18:32PM +0400, Ilya Gladyshev wrote:
> Hi,
>
> I have noticed that progress reporting for CREATE INDEX of partitioned
> tables seems to be working poorly for nested partitioned tables. In
> particular, it overwrites total and done partitions count when it
> recurses down to child partitioned tables and it only reports top-level
> partitions. So it's not hard to see something like this during CREATE
> INDEX now:
>
> postgres=# select partitions_total, partitions_done from
> pg_stat_progress_create_index ;
> partitions_total | partitions_done
> --+-
> 1 | 2
> (1 row)
Yeah. I didn't verify, but it looks like this is a bug going to back to
v12. As you said, when called recursively, DefineIndex() clobbers the
number of completed partitions.
Maybe DefineIndex() could flatten the list of partitions. But I don't
think that can work easily with iteration rather than recursion.
Could you check what I've written as a counter-proposal ?
As long as we're changing partitions_done to include nested
sub-partitions, it seems to me like we should exclude intermediate
"catalog-only" partitioned indexes, and count only physical leaf
partitions. Should it alo exclude any children with matching indexes,
which will also be catalog-only changes? Probably not.
The docs say:
|When creating an index on a partitioned table, this column is set to the
|total number of partitions on which the index is to be created. This
|field is 0 during a REINDEX.
> I changed current behaviour to report the total number of partitions in
> the inheritance tree and fixed recursion in the attached patch. I used
> a static variable to keep the counter to avoid ABI breakage of
> DefineIndex, so that we could backpatch this to previous versions.
I wrote a bunch of assertions for this, which seems to have uncovered an
similar issue with COPY progress reporting, dating to 8a4f618e7. I'm
not sure the assertions are okay. I imagine they may break other
extensions, as with file_fdw.
--
Justin
>From b8077babf9a101f9d1bf41dd1ad866d2ea38b603 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sat, 10 Dec 2022 16:17:50 -0600
Subject: [PATCH] fix progress reporting of nested, partitioned indexes..
---
src/backend/commands/indexcmds.c | 51 +++--
src/backend/utils/activity/backend_progress.c | 72 +++
2 files changed, 118 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index b5b860c3abf..6caa1f94ed7 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -482,6 +482,26 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress)
}
}
+/*
+ * Count the number of direct and indirect leaf partitions, excluding foreign
+ * tables.
+ */
+static int
+num_leaf_partitions(Oid relid)
+{
+ int nleaves = 0;
+ List *childs = find_all_inheritors(relid, NoLock, NULL);
+ ListCell *lc;
+
+ foreach(lc, childs)
+ {
+ Oid partrelid = lfirst_oid(lc);
+ if (RELKIND_HAS_STORAGE(get_rel_relkind(partrelid)))
+ nleaves++;
+ }
+
+ return nleaves;
+}
/*
* DefineIndex
@@ -1212,14 +1232,22 @@ DefineIndex(Oid relationId,
partdesc = RelationGetPartitionDesc(rel, true);
if ((!stmt->relation || stmt->relation->inh) && partdesc->nparts > 0)
{
- int nparts = partdesc->nparts;
+ int nparts = partdesc->nparts; /* only direct children */
+ int nparts_done = 0; /* direct and indirect children */
Oid *part_oids = palloc_array(Oid, nparts);
bool invalidate_parent = false;
Relation parentIndex;
TupleDesc parentDesc;
- pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
- nparts);
+ if (!OidIsValid(parentIndexId))
+ {
+/*
+ * Report the number of leaf partitions (excluding foreign
+ * tables), not just direct children.
+ */
+pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
+ num_leaf_partitions(relationId));
+ }
/* Make a local copy of partdesc->oids[], just for safety */
memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
@@ -1431,8 +1459,21 @@ DefineIndex(Oid relationId,
child_save_sec_context);
}
-pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
- i + 1);
+if (!OidIsValid(parentIndexId))
+{
+ /*
+ * Report progress only when processing top-level indexes.
+ * When recursing, the called functions wouldn't know the
+ * current number of partitions which were processed.
+ * After recursing, increment by the number of children.
+ * This works also for physical/leaf partitions.
+ */
+ nparts_done += num_leaf_partitions(childRelid);
+
+ pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
+ nparts_done);
+}
+
free_attrmap(attmap);
}
diff --git
Re: GetNewObjectId question
Michael Paquier writes: > On Sat, Dec 10, 2022 at 07:11:13PM -0500, Tom Lane wrote: >> Yeah, that looks like somebody didn't read the memo. >> Want to submit a patch? > The comment has been added in e3ce2de but the call originates from > 6566133, so that's a HEAD-only issue. Ah, good that the bug hasn't made it to a released version yet. But that comment is *way* older than e3ce2de; it's been touched a couple of times, but it dates to 721e53785 AFAICS. regards, tom lane
Re: [PROPOSAL] new diagnostic items for the dynamic sql
2022年8月3日(水) 7:56 Tom Lane : > > Jacob Champion writes: > > ...and Dinesh's email has just bounced back undelivered. :( > > > Anybody interested in running with this? If no one speaks up, I think we > > should return this as "needs more interest" before the next CF starts. > > Meh ... the last versions of the patch were far too invasive for a > use-case that seemed pretty hypothetical to begin with. It was never > explained why somebody would be trying to debug dynamic SQL without > use of the reporting that already exists: > > regression=# do $$ begin > regression$# execute 'SELECT 1 JOIN SELECT 2'; > regression$# end $$; > ERROR: syntax error at or near "SELECT" > LINE 1: SELECT 1 JOIN SELECT 2 > ^ > QUERY: SELECT 1 JOIN SELECT 2 > CONTEXT: PL/pgSQL function inline_code_block line 2 at EXECUTE > > psql didn't provide that query text and cursor position out of thin air. > > Now admittedly, what it did base that on is the PG_DIAG_INTERNAL_QUERY and > PG_DIAG_INTERNAL_POSITION fields of the error report, and the fact that > those aren't available to plpgsql error trapping logic is arguably a > deficiency. It's not a big deficiency, because what an EXCEPTION clause > probably ought to do in a case like this is just re-RAISE, which will > preserve those fields in the eventual client error report. But maybe > it's worth fixing. > > I think the real reason this patch stalled is that Pavel wanted the > goal posts moved into the next stadium. Rather than just duplicate > the functionality available in the wire protocol, he wanted some other > definition entirely, hiding the fact that not every error report has > those fields. There isn't infrastructure for that, and I doubt that > this patch is enough to create it, even if there were consensus that > the definition is right. If we were to go forward, I'd recommend > reverting to a wire-protocol-equivalent definition, and just returning > NULL in the cases where the data isn't supplied. I think given this patch has gone nowhere for the past year, we can mark it as returned with feedback. If there's potential for the items mentioned by Tom and someone wants to run with them, that'd be better done with a fresh entry, maybe referencing this one. Regards Ian Barwick
Re: Allow parallel plan for referential integrity checks?
2022年7月26日(火) 20:58 Frédéric Yhuel : > > > > On 4/14/22 14:25, Frédéric Yhuel wrote: > > > > > > On 3/19/22 01:57, Imseih (AWS), Sami wrote: > >> I looked at your patch and it's a good idea to make foreign key > >> validation > >> use parallel query on large relations. > >> > >> It would be valuable to add logging to ensure that the ActiveSnapshot > >> and TransactionSnapshot > >> is the same for the leader and the workers. This logging could be > >> tested in the TAP test. > >> > >> Also, inside RI_Initial_Check you may want to set max_parallel_workers to > >> max_parallel_maintenance_workers. > >> > >> Currently the work_mem is set to maintenance_work_mem. This will also > >> require > >> a doc change to call out. > >> > >> /* > >> * Temporarily increase work_mem so that the check query can be > >> executed > >> * more efficiently. It seems okay to do this because the query > >> is simple > >> * enough to not use a multiple of work_mem, and one typically > >> would not > >> * have many large foreign-key validations happening > >> concurrently. So > >> * this seems to meet the criteria for being considered a > >> "maintenance" > >> * operation, and accordingly we use maintenance_work_mem. > >> However, we > >> > > > > Hello Sami, > > > > Thank you for your review! > > > > I will try to do as you say, but it will take time, since my daily job > > as database consultant takes most of my time and energy. > > > > Hi, > > As suggested by Jacob, here is a quick message to say that I didn't find > enough time to work further on this patch, but I didn't completely > forget it either. I moved it to the next commitfest. Hopefully I will > find enough time and motivation in the coming months :-) Hi Frédéric This patch has been carried forward for a couple more commitfests since your message; do you think you'll be able to work on it further during this release cycle? Thanks Ian Barwick
Re: [RFC] Add jit deform_counter
Hi ne 11. 12. 2022 v 1:14 odesílatel Ian Lawrence Barwick napsal: > 2022年6月12日(日) 18:14 Dmitry Dolgov <9erthali...@gmail.com>: > > > > Hi, > > > > I've noticed that JIT performance counter generation_counter seems to > include > > actions, relevant for both jit_expressions and jit_tuple_deforming > options. It > > means one can't directly see what is the influence of jit_tuple_deforming > > alone, which would be helpful when adjusting JIT options. To make it > better a > > new counter can be introduced, does it make sense? > > Hi Pavel > > I see you are added as reviewer in the CF app; have you been able to take > a look > at this? > I hope so yes Regards Pavel > > Regards > > Ian Barwick >
Re: [BUG] Logical replica crash if there was an error in a function.
On 07.12.2022 21:03, Andres Freund wrote:
This CF entry causes tests to fail on all platforms:
https://cirrus-ci.com/build/5755408111894528
E.g.
https://api.cirrus-ci.com/v1/artifact/task/5298457144459264/testrun/build/testrun/subscription/100_bugs/log/regress_log_100_bugs
Begin standard error
psql::1: NOTICE: dropped replication slot "sub1" on publisher
End standard error
timed out waiting for match: ERROR: relation "error_name" does not exist at
character at /tmp/cirrus-ci-build/src/test/subscription/t/100_bugs.pl line 115.
Greetings,
Andres Freund
Thank you for reminding!
There was a conflict when applying v2 on current master.
Rebased v3 is attached.
Best wishes!
--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companycommit 07eaa674953ed700a53174410a6e1eb81151d7e8
Author: Anton A. Melnikov
Date: Sun Dec 11 06:26:03 2022 +0300
Add test for syntax error in the function in a a logical replication
worker. See dea834938.
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index 7b3cd66be5..0bf4fdfa47 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -69,6 +69,51 @@ $node_publisher->wait_for_catchup('sub1');
pass('index predicates do not cause crash');
+# https://www.postgresql.org/message-id/flat/adf0452f-8c6b-7def-d35e-ab516c80088e%40inbox.ru
+
+# The bug was that when a syntax error occurred in a SQL-language or PL/pgSQL-language
+# CREATE FUNCTION or DO command executed in a logical replication worker,
+# we'd suffer a null pointer dereference or assertion failure.
+
+$node_subscriber->safe_psql('postgres', q{
+ create or replace procedure rebuild_test(
+ ) as
+ $body$
+ declare
+ l_code text:= E'create or replace function public.test_selector(
+ ) returns setof public.tab1 as
+ \$body\$
+ select * from error_name
+ \$body\$ language sql;';
+ begin
+ execute l_code;
+ perform public.test_selector();
+ end
+ $body$ language plpgsql;
+ create or replace function test_trg()
+ returns trigger as
+ $body$
+ declare
+ begin
+ call public.rebuild_test();
+ return NULL;
+ end
+ $body$ language plpgsql;
+ create trigger test_trigger after insert on tab1 for each row
+execute function test_trg();
+ alter table tab1 enable replica trigger test_trigger;
+});
+
+# This would crash on the subscriber if not fixed
+$node_publisher->safe_psql('postgres', "INSERT INTO tab1 VALUES (3, 4)");
+
+my $result = $node_subscriber->wait_for_log(
+ "ERROR: relation \"error_name\" does not exist at character"
+);
+
+ok($result,
+ "ERROR: Logical decoding doesn't fail on function error");
+
# We'll re-use these nodes below, so drop their replication state.
# We don't bother to drop the tables though.
$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub1");
Re: Raising the SCRAM iteration count
On Sun, Dec 11, 2022 at 12:46:23AM +0100, Daniel Gustafsson wrote:
> SCRAM with an iteration count of 1 still provides a lot of benefits over md5,
> so if we can make those comparable in performance then that could be a way
> forward (with the tradeoffs properly documented).
Okay, it looks like there is a wish to make that configurable anyway,
and I have a few comments about that.
{"scram_iteration_count", PGC_SUSET, CONN_AUTH_AUTH,
+ gettext_noop("Sets the iteration count for SCRAM secret
generation."),
+ NULL,
+ GUC_NOT_IN_SAMPLE | GUC_SUPERUSER_ONLY
+ },
Shouldn't this be user-settable as a PGC_USERSET rather than
PGC_SUSET which would limit its updates to superusers?
As shaped, the GUC would not benefit to \password, and we should not
encourage users to give a raw password over the wire if possible if
they wish to compute a verifier with a given interation number.
Hence, wouldn't it be better to mark it as GUC_REPORT, and store its
status in pg_conn@libpq-int.h in the same fashion as
default_transaction_read_only and hot_standby? This way,
PQencryptPasswordConn() would be able to feed on it automatically
rather than always assume the default implied by
pg_fe_scram_build_secret().
--
Michael
signature.asc
Description: PGP signature
Re: GetNewObjectId question
On Sat, Dec 10, 2022 at 07:11:13PM -0500, Tom Lane wrote: > Yeah, that looks like somebody didn't read the memo. > Want to submit a patch? The comment has been added in e3ce2de but the call originates from 6566133, so that's a HEAD-only issue. -- Michael signature.asc Description: PGP signature
Re: -Wunreachable-code-generic-assoc warnings on elver
Thomas Munro writes: > On Sat, Dec 10, 2022 at 3:50 PM Tom Lane wrote: >> Recently, buildfarm member elver has started spewing literally >> thousands of $SUBJECT: > It was using LLVM and clang 15 for the JIT support (the base compiler > cc is clang 13 on this system, but CLANG is set to 15 for the .bc > files, to match the LLVM version). Apparently clang 15 started > issuing a new warning for math.h. That header has since been > adjusted[1] to fix that, but that's not going to show up in the > release that elver's using for a while. I've told it to use > LLVM/clang 14 instead for now; let's see if that helps. Looks like that did the trick, thanks! regards, tom lane
Re: [RFC] Add jit deform_counter
2022年6月12日(日) 18:14 Dmitry Dolgov <9erthali...@gmail.com>: > > Hi, > > I've noticed that JIT performance counter generation_counter seems to include > actions, relevant for both jit_expressions and jit_tuple_deforming options. It > means one can't directly see what is the influence of jit_tuple_deforming > alone, which would be helpful when adjusting JIT options. To make it better a > new counter can be introduced, does it make sense? Hi Pavel I see you are added as reviewer in the CF app; have you been able to take a look at this? Regards Ian Barwick
Re: GetNewObjectId question
Maciek Sakrejda writes: > While browsing through varsup.c, I noticed this comment on GetNewObjectId: > * Hence, this routine should generally not be used directly. The only direct > * callers should be GetNewOidWithIndex() and GetNewRelFileNumber() in > * catalog/catalog.c. > But AddRoleMems in user.c appears to also call the function directly: > /* get an OID for the new row and insert it */ > objectId = GetNewObjectId(); Yeah, that looks like somebody didn't read the memo. Want to submit a patch? regards, tom lane
Re: pg_rewind race condition just after promotion
2021年11月9日(火) 20:31 Daniel Gustafsson : > > > On 14 Jul 2021, at 14:03, Aleksander Alekseev > > wrote: > > > > The following review has been posted through the commitfest application: > > make installcheck-world: tested, passed > > Implements feature: tested, passed > > Spec compliant: tested, passed > > Documentation:tested, passed > > > > The v3 patch LGTM. I wonder if we should explicitly say in pg_rewind tests > > that > > they _don't_ have to call `checkpoint`, or otherwise, we will lose the test > > coverage for this scenario. But I don't have a strong opinion on this one. > > > > The new status of this patch is: Ready for Committer > > Heikki, do you have plans to address this patch during this CF? Friendly reminder ping one year on; I haven't looked at this patch in detail but going by the thread contents it seems it should be marked "Ready for Committer"? Moved to the next CF anyway. Regards Ian Barwick
Re: Error-safe user functions
Andrew Dunstan writes: > On 2022-12-10 Sa 14:38, Tom Lane wrote: >> Seeing that SQL/JSON is one of the major drivers of this whole project, >> it seemed a little sad to me that jsonb couldn't manage to implement >> what is required. So I spent a bit of time poking at it. Attached >> is an extended version of your patch that also covers jsonb. > Many thanks for doing this, it looks good. Cool, thanks. Looking at my notes, there's one other loose end I forgot to mention: * Note: pg_unicode_to_server() will throw an error for a * conversion failure, rather than returning a failure * indication. That seems OK. We ought to do something about that, but I'm not sure how hard we ought to work at it. Perhaps it's sufficient to make a variant of pg_unicode_to_server that just returns true/false instead of failing, and add a JsonParseErrorType for "untranslatable character" to let json_errdetail return a reasonably on-point message. We could imagine extending the ErrorSaveContext infrastructure into the encoding conversion modules, and maybe at some point that'll be worth doing, but in this particular context it doesn't seem like we'd be getting a very much better error message. The main thing that we would get from such an extension is a chance to capture the report from report_untranslatable_char. But what that adds is the ability to identify exactly which character couldn't be translated --- and in this use-case there's always just one. regards, tom lane
Re: Raising the SCRAM iteration count
> On 10 Dec 2022, at 01:15, Andres Freund wrote: > On 2022-12-09 11:55:07 +0100, Daniel Gustafsson wrote: >> The attached introduces a scram_iteration_count GUC with a default of 15000 >> (still conservative, from RFC7677) and a minimum of 4096. Since the >> iterations >> are stored per secret it can be altered with backwards compatibility. > > I am extremely doubtful it's a good idea to increase the default (if anything > the opposite). 0.1 seconds is many times the connection establishment > overhead, even over network. I've seen users complain about postgres > connection establishment overhead being high, and it just turned out to be due > to scram - yes, they ended up switching to md5, because that was the only > viable alternative. That's a fair point. For the record I don't think we should raise the default to match 0.1 seconds, but we should make the option available to those who want it. If we provide a GUC for the iteration count which has a lower limit than todays hardcoded value, then maybe we can help workloads with long-lived connections who want increased on-disk safety as well as workloads where low connection establishment is critical (or where the env is constrained like in Heikki's example). > PGPASSWORD=passme pgbench -n -C -f ~/tmp/select.sql -h 127.0.0.1 -T10 -U > passme > > md5: tps = 158.577609 > scram: tps = 38.196362 Lowering the minimum for scram_iteration_count I tried out the patch on a set of iteration counts of interest. Values are averaged over three runs, using the same pgbench setup you had above with basically a noop select.sql. The relative difference between the values are way off from your results, but I haven't done much digging to figure that out yet (different OpenSSL versions might be one factor). md5: tps = 154.052690 scram 1: tps = 150.060285 scram 1024: tps = 138.191224 scram 4096: tps = 115.197533 scram 15000: tps = 75.156399 For the fun of it, 10 iterations yields tps = 20.822393. SCRAM with an iteration count of 1 still provides a lot of benefits over md5, so if we can make those comparable in performance then that could be a way forward (with the tradeoffs properly documented). -- Daniel Gustafsson https://vmware.com/
GetNewObjectId question
While browsing through varsup.c, I noticed this comment on GetNewObjectId: * Hence, this routine should generally not be used directly. The only direct * callers should be GetNewOidWithIndex() and GetNewRelFileNumber() in * catalog/catalog.c. But AddRoleMems in user.c appears to also call the function directly: /* get an OID for the new row and insert it */ objectId = GetNewObjectId(); new_record[Anum_pg_auth_members_oid - 1] = objectId; tuple = heap_form_tuple(pg_authmem_dsc, new_record, new_record_nulls); CatalogTupleInsert(pg_authmem_rel, tuple); I'm not sure if that call is right, but this seems inconsistent. Should that caller be using GetNewOidWithIndex instead? Or should the comment be updated? Thanks, Maciek
Re: Error-safe user functions
On 2022-12-10 Sa 14:38, Tom Lane wrote: > Andrew Dunstan writes: >> OK, json is a fairly easy case, see attached. But jsonb is a different >> kettle of fish. Both the semantic routines called by the parser and the >> subsequent call to JsonbValueToJsonb() can raise errors. These are >> pretty much all about breaking various limits (for strings, objects, >> arrays). There's also a call to numeric_in, but I assume that a string >> that's already parsed as a valid json numeric literal won't upset >> numeric_in. > Um, nope ... > > regression=# select '1e100'::jsonb; > ERROR: value overflows numeric format > LINE 1: select '1e100'::jsonb; >^ Oops, yeah. >> Many of these occur several calls down the stack, so >> adjusting everything to deal with them would be fairly invasive. Perhaps >> we could instead document that this class of input error won't be >> trapped, at least for jsonb. > Seeing that SQL/JSON is one of the major drivers of this whole project, > it seemed a little sad to me that jsonb couldn't manage to implement > what is required. So I spent a bit of time poking at it. Attached > is an extended version of your patch that also covers jsonb. > > The main thing I soon realized is that the JsonSemAction API is based > on the assumption that semantic actions will report errors by throwing > them. This is a bit schizophrenic considering the parser itself carefully > hands back error codes instead of throwing anything (excluding palloc > failures of course). What I propose in the attached is that we change > that API so that action functions return JsonParseErrorType, and add > an enum value denoting "I already logged a suitable error, so you don't > have to". It was a little tedious to modify all the existing functions > that way, but not hard. Only the ones used by jsonb_in need to do > anything except "return JSON_SUCCESS", at least for now. Many thanks for doing this, it looks good. > I have not done anything here about errors within JsonbValueToJsonb. > There would need to be another round of API-extension in that area > if we want to be able to trap its errors. As you say, those are mostly > about exceeding implementation size limits, so I suppose one could argue > that they are not so different from palloc failure. It's still annoying. > If people are good with the changes attached, I might take a look at > that. > > Awesome. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Speedup generation of command completion tags
Hi,
On 2022-12-10 20:32:06 +1300, David Rowley wrote:
> @@ -20,13 +20,14 @@
> typedef struct CommandTagBehavior
> {
> const char *name;
> + const uint8 namelen;
Perhaps worth adding a comment noting that namelen is the length without the
null byte?
> +static inline Size
> +make_completion_tag(char *buff, const QueryCompletion *qc,
> + bool force_undecorated_output)
> +{
> + CommandTag tag = qc->commandTag;
> + Sizetaglen;
> + const char *tagname = GetCommandTagNameAndLen(tag, );
> + char *bufp;
> +
> + /*
> + * We assume the tagname is plain ASCII and therefore requires no
> encoding
> + * conversion.
> + */
> + memcpy(buff, tagname, taglen + 1);
> + bufp = buff + taglen;
> +
> + /* ensure that the tagname isn't long enough to overrun the buffer */
> + Assert(taglen <= COMPLETION_TAG_BUFSIZE - MAXINT8LEN - 4);
> +
> + /*
> + * In PostgreSQL versions 11 and earlier, it was possible to create a
> + * table WITH OIDS. When inserting into such a table, INSERT used to
> + * include the Oid of the inserted record in the completion tag. To
> + * maintain compatibility in the wire protocol, we now write a "0" (for
> + * InvalidOid) in the location where we once wrote the new record's Oid.
> + */
> + if (command_tag_display_rowcount(tag) && !force_undecorated_output)
This does another external function call to cmdtag.c...
What about moving make_completion_tag() to cmdtag.c? Then we could just get
the entire CommandTagBehaviour struct at once. It's not super pretty to pass
QueryCompletion to a routine in cmdtag.c, but it's not awful. And if we deem
it problematic, we could just pass qc->commandTag, qc->nprocessed as a
separate arguments.
I wonder if any of the other GetCommandTagName() would benefit noticably from
not having to compute the length. I guess the calls
set_ps_display(GetCommandTagName()) calls in exec_simple_query() and
exec_execute_message() might, although set_ps_display() isn't exactly zero
overhead. But I do see it show up as a few percent in profiles, with the
biggest contributor being the call to strlen.
Greetings,
Andres Freund
Re: Error-safe user functions
Corey Huinker writes:
> On Sat, Dec 10, 2022 at 9:20 AM Tom Lane wrote:
>> A fallback we can offer to anyone with such a problem is "write a
>> plpgsql function and wrap the potentially-failing bit in an exception
>> block". Then they get to pay the cost of the subtransaction, while
>> we're not imposing one on everybody else.
> That exception block will prevent parallel plans. I'm not saying it isn't
> the best way forward for us, but wanted to make that side effect clear.
Hmm. Apropos of that, I notice that domain_in is marked PARALLEL SAFE,
which seems like a bad idea if it could invoke not-so-parallel-safe
expressions. Do we need to mark it less safe, and if so how much less?
Anyway, assuming that people are okay with the Not Our Problem approach,
the patch is pretty trivial, as attached. I started to write an addition
to the CREATE DOMAIN man page recommending that domain CHECK constraints
not throw errors, but couldn't get past the bare recommendation. Normally
I'd want to explain such a thing along the lines of "For example, X won't
work" ... but we don't yet have any committed features that depend on
this. I'm inclined to leave it like that for now. If we don't remember
to fix it once we do have some features, I'm sure somebody will ask a
question about it eventually.
regards, tom lane
diff --git a/doc/src/sgml/ref/create_domain.sgml b/doc/src/sgml/ref/create_domain.sgml
index 82a0b87492..73f9f28d6c 100644
--- a/doc/src/sgml/ref/create_domain.sgml
+++ b/doc/src/sgml/ref/create_domain.sgml
@@ -239,6 +239,11 @@ INSERT INTO tab (domcol) VALUES ((SELECT domcol FROM tab WHERE false));
DOMAIN), adjust the function definition, and re-add the
constraint, thereby rechecking it against stored data.
+
+
+ It's also good practice to ensure that domain CHECK
+ expressions will not throw errors.
+
diff --git a/src/backend/utils/adt/domains.c b/src/backend/utils/adt/domains.c
index 3de0cb01a2..99aeaddb5d 100644
--- a/src/backend/utils/adt/domains.c
+++ b/src/backend/utils/adt/domains.c
@@ -126,9 +126,14 @@ domain_state_setup(Oid domainType, bool binary, MemoryContext mcxt)
* This is roughly similar to the handling of CoerceToDomain nodes in
* execExpr*.c, but we execute each constraint separately, rather than
* compiling them in-line within a larger expression.
+ *
+ * If escontext points to an ErrorStateContext, any failures are reported
+ * there, otherwise they are ereport'ed. Note that we do not attempt to do
+ * soft reporting of errors raised during execution of CHECK constraints.
*/
static void
-domain_check_input(Datum value, bool isnull, DomainIOData *my_extra)
+domain_check_input(Datum value, bool isnull, DomainIOData *my_extra,
+ Node *escontext)
{
ExprContext *econtext = my_extra->econtext;
ListCell *l;
@@ -144,11 +149,14 @@ domain_check_input(Datum value, bool isnull, DomainIOData *my_extra)
{
case DOM_CONSTRAINT_NOTNULL:
if (isnull)
- ereport(ERROR,
+{
+ errsave(escontext,
(errcode(ERRCODE_NOT_NULL_VIOLATION),
errmsg("domain %s does not allow null values",
format_type_be(my_extra->domain_type)),
errdatatype(my_extra->domain_type)));
+ goto fail;
+}
break;
case DOM_CONSTRAINT_CHECK:
{
@@ -179,13 +187,16 @@ domain_check_input(Datum value, bool isnull, DomainIOData *my_extra)
econtext->domainValue_isNull = isnull;
if (!ExecCheck(con->check_exprstate, econtext))
- ereport(ERROR,
+ {
+ errsave(escontext,
(errcode(ERRCODE_CHECK_VIOLATION),
errmsg("value for domain %s violates check constraint \"%s\"",
format_type_be(my_extra->domain_type),
con->name),
errdomainconstraint(my_extra->domain_type,
con->name)));
+ goto fail;
+ }
break;
}
default:
@@ -200,6 +211,7 @@ domain_check_input(Datum value, bool isnull, DomainIOData *my_extra)
* per-tuple memory. This avoids leaking non-memory resources, if
* anything in the expression(s) has any.
*/
+fail:
if (econtext)
ReScanExprContext(econtext);
}
@@ -213,6 +225,7 @@ domain_in(PG_FUNCTION_ARGS)
{
char *string;
Oid domainType;
+ Node *escontext = fcinfo->context;
DomainIOData *my_extra;
Datum value;
@@ -245,15 +258,18 @@ domain_in(PG_FUNCTION_ARGS)
/*
* Invoke the base type's typinput procedure to convert the data.
*/
- value = InputFunctionCall(_extra->proc,
- string,
- my_extra->typioparam,
- my_extra->typtypmod);
+ if (!InputFunctionCallSafe(_extra->proc,
+ string,
+ my_extra->typioparam,
+ my_extra->typtypmod,
+ escontext,
+ ))
+ PG_RETURN_NULL();
/*
* Do the necessary checks to ensure it's a valid domain value.
*/
- domain_check_input(value, (string == NULL), my_extra);
+ domain_check_input(value, (string == NULL), my_extra, escontext);
if (string ==
Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
On Sat, Dec 10, 2022 at 12:07:12PM -0800, Jeff Davis wrote: > It seems like the discussion on VACUUM/CLUSTER/REINDEX privileges is > happening in the other thread. What would you like to accomplish in > this thread? Given the feedback in the other thread [0], I was planning to rewrite this patch to create a MAINTAIN privilege and a pg_maintain_all_tables predefined role that allowed VACUUM, ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX. [0] https://postgr.es/m/20221206193606.GB3078082%40nathanxps13 -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: -Wunreachable-code-generic-assoc warnings on elver
On Sat, Dec 10, 2022 at 3:50 PM Tom Lane wrote: > Recently, buildfarm member elver has started spewing literally > thousands of $SUBJECT: > > elver | 2022-12-10 01:17:29 | > ../../src/include/utils/float.h:223:33: warning: due to lvalue conversion of > the controlling expression, association of type 'volatile float' will never > be selected because it is qualified [-Wunreachable-code-generic-assoc] > elver | 2022-12-10 01:17:29 | > ../../src/include/utils/float.h:223:33: warning: due to lvalue conversion of > the controlling expression, association of type 'volatile double' will never > be selected because it is qualified [-Wunreachable-code-generic-assoc] > elver | 2022-12-10 01:17:29 | > ../../src/include/utils/float.h:223:33: warning: due to lvalue conversion of > the controlling expression, association of type 'volatile long double' will > never be selected because it is qualified [-Wunreachable-code-generic-assoc] > [ etc etc, about 9200 times per build ] > > I have no idea what that means, and consulting the clang documentation > didn't leave me much wiser. I do see that a lot of these seem to be > associated with isnan() calls, which makes me guess that clang does > not play nice with however isnan() is declared on that box. It was using LLVM and clang 15 for the JIT support (the base compiler cc is clang 13 on this system, but CLANG is set to 15 for the .bc files, to match the LLVM version). Apparently clang 15 started issuing a new warning for math.h. That header has since been adjusted[1] to fix that, but that's not going to show up in the release that elver's using for a while. I've told it to use LLVM/clang 14 instead for now; let's see if that helps. [1] https://github.com/freebsd/freebsd-src/commit/8432a5a4fa3c4f34acf6136a9077b9ab7bbd723e
Re: ExecRTCheckPerms() and many prunable partitions (checkAsUser)
On Tue, Nov 29, 2022 at 10:37:56PM +0900, Amit Langote wrote:
> 0002 contains changes that has to do with changing how we access
> checkAsUser in some foreign table planning/execution code sites.
> Thought it might be better to describe it separately too.
This was committed as 599b33b94:
Stop accessing checkAsUser via RTE in some cases
Which does this in a couple places in selfuncs.c:
if (!vardata->acl_ok &&
root->append_rel_array != NULL)
{
AppendRelInfo *appinfo;
Index varno = index->rel->relid;
appinfo = root->append_rel_array[varno];
while (appinfo &&
planner_rt_fetch(appinfo->parent_relid,
root)->rtekind ==
RTE_RELATION)
{
varno = appinfo->parent_relid;
appinfo = root->append_rel_array[varno];
}
if (varno != index->rel->relid)
{
/* Repeat access check on this rel */
rte = planner_rt_fetch(varno, root);
Assert(rte->rtekind == RTE_RELATION);
- userid = rte->checkAsUser ?
rte->checkAsUser : GetUserId();
+ userid = OidIsValid(onerel->userid) ?
+ onerel->userid : GetUserId();
vardata->acl_ok =
rte->securityQuals == NIL &&
(pg_class_aclcheck(rte->relid,
userid,
ACL_SELECT) ==
ACLCHECK_OK);
}
}
The original code rechecks rte->checkAsUser with the rte of the parent
rel. The patch changed to access onerel instead, but that's not updated
after looping to find the parent.
Is that okay ? It doesn't seem intentional, since "userid" is still
being recomputed, but based on onerel, which hasn't changed. The
original intent (since 553d2ec27) is to recheck the parent's
"checkAsUser".
It seems like this would matter for partitioned tables, when the
partition isn't readable, but its parent is, and accessed via a view
owned by another user?
--
Justin
Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
On Thu, 2022-12-08 at 10:37 -0800, Nathan Bossart wrote: > 0001 makes it possible to > grant CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX. 0002 adds > predefined roles that allow performing these commands on all > relations. Regarding the pg_refresh_all_matview predefined role, I don't think it's a good idea. Refreshing a materialized view doesn't seem like an administrative action to me. First, it's unbounded in time, so the admin would need to be careful to have a timeout. Second, the freshness of a materialized view seems very specific to the application, rather than something that an admin would have a blanket policy about. Thirdly, there's not a lot of information the admin could use to make decisions about when to refresh (as opposed to VACUUM/CLUSTER/REINDEX, where the stats are helpful). But I'm fine with having a grantable privilege to refresh a materialized view. It seems like the discussion on VACUUM/CLUSTER/REINDEX privileges is happening in the other thread. What would you like to accomplish in this thread? -- Jeff Davis PostgreSQL Contributor Team - AWS
Re: Error-safe user functions
Andrew Dunstan writes:
> OK, json is a fairly easy case, see attached. But jsonb is a different
> kettle of fish. Both the semantic routines called by the parser and the
> subsequent call to JsonbValueToJsonb() can raise errors. These are
> pretty much all about breaking various limits (for strings, objects,
> arrays). There's also a call to numeric_in, but I assume that a string
> that's already parsed as a valid json numeric literal won't upset
> numeric_in.
Um, nope ...
regression=# select '1e100'::jsonb;
ERROR: value overflows numeric format
LINE 1: select '1e100'::jsonb;
^
> Many of these occur several calls down the stack, so
> adjusting everything to deal with them would be fairly invasive. Perhaps
> we could instead document that this class of input error won't be
> trapped, at least for jsonb.
Seeing that SQL/JSON is one of the major drivers of this whole project,
it seemed a little sad to me that jsonb couldn't manage to implement
what is required. So I spent a bit of time poking at it. Attached
is an extended version of your patch that also covers jsonb.
The main thing I soon realized is that the JsonSemAction API is based
on the assumption that semantic actions will report errors by throwing
them. This is a bit schizophrenic considering the parser itself carefully
hands back error codes instead of throwing anything (excluding palloc
failures of course). What I propose in the attached is that we change
that API so that action functions return JsonParseErrorType, and add
an enum value denoting "I already logged a suitable error, so you don't
have to". It was a little tedious to modify all the existing functions
that way, but not hard. Only the ones used by jsonb_in need to do
anything except "return JSON_SUCCESS", at least for now.
(I wonder if pg_verifybackup's parse_manifest.c could use a second
look at how it's handling errors, given this API. I didn't study it
closely.)
I have not done anything here about errors within JsonbValueToJsonb.
There would need to be another round of API-extension in that area
if we want to be able to trap its errors. As you say, those are mostly
about exceeding implementation size limits, so I suppose one could argue
that they are not so different from palloc failure. It's still annoying.
If people are good with the changes attached, I might take a look at
that.
regards, tom lane
diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index fee2ffb55c..e6896eccfe 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -81,9 +81,10 @@ json_in(PG_FUNCTION_ARGS)
/* validate it */
lex = makeJsonLexContext(result, false);
- pg_parse_json_or_ereport(lex, );
+ if (!pg_parse_json_or_errsave(lex, , fcinfo->context))
+ PG_RETURN_NULL();
- /* Internal representation is the same as text, for now */
+ /* Internal representation is the same as text */
PG_RETURN_TEXT_P(result);
}
@@ -1337,7 +1338,7 @@ json_typeof(PG_FUNCTION_ARGS)
/* Lex exactly one token from the input and check its type. */
result = json_lex(lex);
if (result != JSON_SUCCESS)
- json_ereport_error(result, lex);
+ json_errsave_error(result, lex, NULL);
tok = lex->token_type;
switch (tok)
{
diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index 9e14922ec2..7c1e5e6144 100644
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -33,6 +33,7 @@ typedef struct JsonbInState
{
JsonbParseState *parseState;
JsonbValue *res;
+ Node *escontext;
} JsonbInState;
/* unlike with json categories, we need to treat json and jsonb differently */
@@ -61,15 +62,15 @@ typedef struct JsonbAggState
Oid val_output_func;
} JsonbAggState;
-static inline Datum jsonb_from_cstring(char *json, int len);
-static size_t checkStringLen(size_t len);
-static void jsonb_in_object_start(void *pstate);
-static void jsonb_in_object_end(void *pstate);
-static void jsonb_in_array_start(void *pstate);
-static void jsonb_in_array_end(void *pstate);
-static void jsonb_in_object_field_start(void *pstate, char *fname, bool isnull);
+static inline Datum jsonb_from_cstring(char *json, int len, Node *escontext);
+static bool checkStringLen(size_t len, Node *escontext);
+static JsonParseErrorType jsonb_in_object_start(void *pstate);
+static JsonParseErrorType jsonb_in_object_end(void *pstate);
+static JsonParseErrorType jsonb_in_array_start(void *pstate);
+static JsonParseErrorType jsonb_in_array_end(void *pstate);
+static JsonParseErrorType jsonb_in_object_field_start(void *pstate, char *fname, bool isnull);
static void jsonb_put_escaped_value(StringInfo out, JsonbValue *scalarVal);
-static void jsonb_in_scalar(void *pstate, char *token, JsonTokenType tokentype);
+static JsonParseErrorType jsonb_in_scalar(void *pstate, char *token, JsonTokenType tokentype);
static void jsonb_categorize_type(Oid typoid,
JsonbTypeCategory *tcategory,
Oid
Infinite Interval
Hi all, There have been multiple threads in the past discussing infinite intervals: https://www.postgresql.org/message-id/flat/4EB095C8.1050703%40agliodbs.com https://www.postgresql.org/message-id/flat/200101241913.f0OJDUu45423%40hub.org https://www.postgresql.org/message-id/flat/CANP8%2BjKTxQh4Mj%2BU3mWO3JHYb11SeQX9FW8SENrGbTdVxu6NNA%40mail.gmail.com As well as an entry in the TODO list: https://wiki.postgresql.org/wiki/Todo#Dates_and_Times However, it doesn't seem like this was ever implemented. Is there still any interest in this feature? If so, I'd like to try and implement it. The proposed design from the most recent thread was to reserve INT32_MAX months for infinity and INT32_MIN months for negative infinity. As pointed out in the thread, these are currently valid non-infinite intervals, but they are out of the documented range. Thanks, Joe Koshakow
Re: Error-safe user functions
On Sat, Dec 10, 2022 at 9:20 AM Tom Lane wrote: > Alvaro Herrera writes: > > On 2022-Dec-09, Tom Lane wrote: > >> ... So I think it might be > >> okay to say "if you want soft error treatment for a domain, > >> make sure its check constraints don't throw errors". > > > I think that's fine. If the user does, say "CHECK (value > 0)" and that > > results in a soft error, that seems to me enough support for now. If > > they want to do something more elaborate, they can write C functions. > > Maybe eventually we'll want to offer some other mechanism that doesn't > > require C, but let's figure out what the requirements are. I don't > > think we know that, at this point. > > A fallback we can offer to anyone with such a problem is "write a > plpgsql function and wrap the potentially-failing bit in an exception > block". Then they get to pay the cost of the subtransaction, while > we're not imposing one on everybody else. > > regards, tom lane > That exception block will prevent parallel plans. I'm not saying it isn't the best way forward for us, but wanted to make that side effect clear.
Re: Error-safe user functions
so 10. 12. 2022 v 15:35 odesílatel Andrew Dunstan napsal: > > On 2022-12-09 Fr 10:37, Andrew Dunstan wrote: > > I am currently looking at the json types. I think that will be enough to > > let us rework the sql/json patches as discussed a couple of months ago. > > > > OK, json is a fairly easy case, see attached. But jsonb is a different > kettle of fish. Both the semantic routines called by the parser and the > subsequent call to JsonbValueToJsonb() can raise errors. These are > pretty much all about breaking various limits (for strings, objects, > arrays). There's also a call to numeric_in, but I assume that a string > that's already parsed as a valid json numeric literal won't upset > numeric_in. Many of these occur several calls down the stack, so > adjusting everything to deal with them would be fairly invasive. Perhaps > we could instead document that this class of input error won't be > trapped, at least for jsonb. We could still test for well-formed jsonb > input, just as I propose for json. That means that we would not be able > to trap one of these errors in the ON ERROR clause of JSON_TABLE. I > think we can probably live with that. > > Thoughts? > +1 Pavel > > > cheers > > > andrew > > > -- > Andrew Dunstan > EDB: https://www.enterprisedb.com >
Re: Error-safe user functions
On 2022-12-09 Fr 10:37, Andrew Dunstan wrote:
> I am currently looking at the json types. I think that will be enough to
> let us rework the sql/json patches as discussed a couple of months ago.
>
OK, json is a fairly easy case, see attached. But jsonb is a different
kettle of fish. Both the semantic routines called by the parser and the
subsequent call to JsonbValueToJsonb() can raise errors. These are
pretty much all about breaking various limits (for strings, objects,
arrays). There's also a call to numeric_in, but I assume that a string
that's already parsed as a valid json numeric literal won't upset
numeric_in. Many of these occur several calls down the stack, so
adjusting everything to deal with them would be fairly invasive. Perhaps
we could instead document that this class of input error won't be
trapped, at least for jsonb. We could still test for well-formed jsonb
input, just as I propose for json. That means that we would not be able
to trap one of these errors in the ON ERROR clause of JSON_TABLE. I
think we can probably live with that.
Thoughts?
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
From 945ede5fa99f5aa9fb0740fe04303f37aa511528 Mon Sep 17 00:00:00 2001
From: Andrew Dunstan
Date: Sat, 10 Dec 2022 08:18:57 -0500
Subject: [PATCH] adjustments for json_in
---
src/backend/utils/adt/json.c | 7 ---
src/backend/utils/adt/jsonfuncs.c | 26 +-
src/include/utils/jsonfuncs.h | 14 +-
src/test/regress/expected/json.out | 19 +++
src/test/regress/sql/json.sql | 5 +
5 files changed, 54 insertions(+), 17 deletions(-)
diff --git a/src/backend/utils/adt/json.c b/src/backend/utils/adt/json.c
index fee2ffb55c..d5c48c99c3 100644
--- a/src/backend/utils/adt/json.c
+++ b/src/backend/utils/adt/json.c
@@ -81,9 +81,10 @@ json_in(PG_FUNCTION_ARGS)
/* validate it */
lex = makeJsonLexContext(result, false);
- pg_parse_json_or_ereport(lex, );
+ if ( ! pg_parse_json_or_errsave(lex, , fcinfo->context))
+ PG_RETURN_NULL();
- /* Internal representation is the same as text, for now */
+ /* Internal representation is the same as text */
PG_RETURN_TEXT_P(result);
}
@@ -1337,7 +1338,7 @@ json_typeof(PG_FUNCTION_ARGS)
/* Lex exactly one token from the input and check its type. */
result = json_lex(lex);
if (result != JSON_SUCCESS)
- json_ereport_error(result, lex);
+ json_errsave_error(result, lex, NULL);
tok = lex->token_type;
switch (tok)
{
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index bfc3f02a86..1a3cec59cb 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -490,21 +490,28 @@ static void transform_string_values_object_field_start(void *state, char *fname,
static void transform_string_values_array_element_start(void *state, bool isnull);
static void transform_string_values_scalar(void *state, char *token, JsonTokenType tokentype);
+
/*
- * pg_parse_json_or_ereport
+ * pg_parse_json_or_errsave
*
* This function is like pg_parse_json, except that it does not return a
* JsonParseErrorType. Instead, in case of any failure, this function will
- * ereport(ERROR).
+ * call errsave(escontext), which will call ereport(ERROR) if escontext is NULL.
+ * Otherwise, returns a boolean indicating success or failure.
*/
-void
-pg_parse_json_or_ereport(JsonLexContext *lex, JsonSemAction *sem)
+bool
+pg_parse_json_or_errsave(JsonLexContext *lex, JsonSemAction *sem,
+ Node *escontext)
{
JsonParseErrorType result;
result = pg_parse_json(lex, sem);
if (result != JSON_SUCCESS)
- json_ereport_error(result, lex);
+ {
+ json_errsave_error(result, lex, escontext);
+ return false;
+ }
+ return true;
}
/*
@@ -608,17 +615,18 @@ jsonb_object_keys(PG_FUNCTION_ARGS)
* Report a JSON error.
*/
void
-json_ereport_error(JsonParseErrorType error, JsonLexContext *lex)
+json_errsave_error(JsonParseErrorType error, JsonLexContext *lex,
+ Node *escontext)
{
if (error == JSON_UNICODE_HIGH_ESCAPE ||
error == JSON_UNICODE_CODE_POINT_ZERO)
- ereport(ERROR,
+ errsave(escontext,
(errcode(ERRCODE_UNTRANSLATABLE_CHARACTER),
errmsg("unsupported Unicode escape sequence"),
errdetail_internal("%s", json_errdetail(error, lex)),
report_json_context(lex)));
else
- ereport(ERROR,
+ errsave(escontext,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("invalid input syntax for type %s", "json"),
errdetail_internal("%s", json_errdetail(error, lex)),
@@ -1260,7 +1268,7 @@ get_array_start(void *state)
error = json_count_array_elements(_state->lex, );
if (error != JSON_SUCCESS)
-json_ereport_error(error, _state->lex);
+json_errsave_error(error, _state->lex, NULL);
if (-_state->path_indexes[lex_level] <= nelements)
_state->path_indexes[lex_level] += nelements;
diff --git a/src/include/utils/jsonfuncs.h b/src/include/utils/jsonfuncs.h
Re: Error-safe user functions
Alvaro Herrera writes: > On 2022-Dec-09, Tom Lane wrote: >> ... So I think it might be >> okay to say "if you want soft error treatment for a domain, >> make sure its check constraints don't throw errors". > I think that's fine. If the user does, say "CHECK (value > 0)" and that > results in a soft error, that seems to me enough support for now. If > they want to do something more elaborate, they can write C functions. > Maybe eventually we'll want to offer some other mechanism that doesn't > require C, but let's figure out what the requirements are. I don't > think we know that, at this point. A fallback we can offer to anyone with such a problem is "write a plpgsql function and wrap the potentially-failing bit in an exception block". Then they get to pay the cost of the subtransaction, while we're not imposing one on everybody else. regards, tom lane
Re: Error-safe user functions
On 2022-Dec-09, Tom Lane wrote: > I think though that it might be okay to just define this as > Not Our Problem. Although we don't seem to try to enforce it, > non-immutable domain check constraints are strongly deprecated > (the CREATE DOMAIN man page says that we assume immutability). > And not throwing errors is something that we usually consider > should ride along with immutability. So I think it might be > okay to say "if you want soft error treatment for a domain, > make sure its check constraints don't throw errors". I think that's fine. If the user does, say "CHECK (value > 0)" and that results in a soft error, that seems to me enough support for now. If they want to do something more elaborate, they can write C functions. Maybe eventually we'll want to offer some other mechanism that doesn't require C, but let's figure out what the requirements are. I don't think we know that, at this point. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "Estoy de acuerdo contigo en que la verdad absoluta no existe... El problema es que la mentira sí existe y tu estás mintiendo" (G. Lama)
Progress report of CREATE INDEX for nested partitioned tables
Hi,
I have noticed that progress reporting for CREATE INDEX of partitioned
tables seems to be working poorly for nested partitioned tables. In
particular, it overwrites total and done partitions count when it
recurses down to child partitioned tables and it only reports top-level
partitions. So it's not hard to see something like this during CREATE
INDEX now:
postgres=# select partitions_total, partitions_done from
pg_stat_progress_create_index ;
partitions_total | partitions_done
--+-
1 | 2
(1 row)
I changed current behaviour to report the total number of partitions in
the inheritance tree and fixed recursion in the attached patch. I used
a static variable to keep the counter to avoid ABI breakage of
DefineIndex, so that we could backpatch this to previous versions.
Thanks,
Ilya Gladyshev
From 342caa3d4ce273b14a6998c20c32b862d2d4c890 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev
Date: Fri, 9 Dec 2022 23:17:29 +0400
Subject: [PATCH] report top parent progress for CREATE INDEX
---
src/backend/commands/indexcmds.c | 36 ++--
1 file changed, 30 insertions(+), 6 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index b5b860c3ab..80557dad8d 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -129,6 +129,12 @@ typedef struct ReindexErrorInfo
char relkind;
} ReindexErrorInfo;
+/*
+ * Used to report the number of partitions,
+ * that were processed during index creation.
+ */
+static int create_index_parts_done;
+
/*
* CheckIndexCompatible
* Determine whether an existing index definition is compatible with a
@@ -1218,8 +1224,18 @@ DefineIndex(Oid relationId,
Relation parentIndex;
TupleDesc parentDesc;
- pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
- nparts);
+ if (!OidIsValid(parentIndexId))
+ {
+List *inhs;
+
+/* Reset counter of done partitions when processing root index */
+create_index_parts_done = 0;
+inhs = find_all_inheritors(relationId, NoLock, NULL);
+/* inheritance tree size without root itself */
+pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
+ list_length(inhs) - 1);
+list_free(inhs);
+ }
/* Make a local copy of partdesc->oids[], just for safety */
memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
@@ -1431,8 +1447,6 @@ DefineIndex(Oid relationId,
child_save_sec_context);
}
-pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
- i + 1);
free_attrmap(attmap);
}
@@ -1465,13 +1479,17 @@ DefineIndex(Oid relationId,
/*
* Indexes on partitioned tables are not themselves built, so we're
- * done here.
+ * done here. If it is a child partitioned table, increment done parts counter.
*/
AtEOXact_GUC(false, root_save_nestlevel);
SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
table_close(rel, NoLock);
if (!OidIsValid(parentIndexId))
pgstat_progress_end_command();
+ else
+ pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
+ ++create_index_parts_done);
+
return address;
}
@@ -1483,9 +1501,15 @@ DefineIndex(Oid relationId,
/* Close the heap and we're done, in the non-concurrent case */
table_close(rel, NoLock);
- /* If this is the top-level index, we're done. */
+ /*
+ * If this is the top-level index, we're done, otherwise, increment
+ * done partition counter.
+ */
if (!OidIsValid(parentIndexId))
pgstat_progress_end_command();
+ else
+ pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
+ ++create_index_parts_done);
return address;
}
--
2.30.2
Re: Minimal logical decoding on standbys
Hi, On 12/8/22 12:07 PM, Drouvot, Bertrand wrote: Hi, On 12/7/22 6:58 PM, Andres Freund wrote: Hi, On 2022-12-07 10:00:25 +0100, Drouvot, Bertrand wrote: Please find attached a new patch series: v27-0001-Add-info-in-WAL-records-in-preparation-for-logic.patch v27-0002-Handle-logical-slot-conflicts-on-standby.patch v27-0003-Allow-logical-decoding-on-standby.patch v27-0004-New-TAP-test-for-logical-decoding-on-standby.patch v27-0005-Doc-changes-describing-details-about-logical-dec.patch v27-0006-Fixing-Walsender-corner-case-with-logical-decodi.patch This failed on cfbot [1]. The tap output [2] has the following bit: [09:48:56.216](5.979s) not ok 26 - cannot read from logical replication slot [09:48:56.223](0.007s) # Failed test 'cannot read from logical replication slot' # at C:/cirrus/src/test/recovery/t/034_standby_logical_decoding.pl line 422. ... Warning: unable to close filehandle GEN150 properly: Bad file descriptor during global destruction. Warning: unable to close filehandle GEN155 properly: Bad file descriptor during global destruction. The "unable to close filehandle" stuff in my experience indicates an IPC::Run process that wasn't ended before the tap test ended. Greetings, Andres Freund [1] https://cirrus-ci.com/task/5092676671373312 [2] https://api.cirrus-ci.com/v1/artifact/task/5092676671373312/testrun/build/testrun/recovery/034_standby_logical_decoding/log/regress_log_034_standby_logical_decoding Thanks for pointing out! Please find attached V29 addressing this "Windows perl" issue: V29 changes the way the slot invalidation is tested and adds a "handle->finish". That looks ok now (I launched several successful consecutive tests on my enabled cirrus-ci repository). V29 differs from V28 only in 0004 to workaround the above "Windows perl" issue. Regards, Attaching V30, mandatory rebase due to 66dcb09246. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.comFrom d592f165e8fdd7ecc3dd4e99a443572844ea0cba Mon Sep 17 00:00:00 2001 From: bdrouvotAWS Date: Sat, 10 Dec 2022 07:23:39 + Subject: [PATCH v30 6/6] Fixing Walsender corner case with logical decoding on standby. The problem is that WalSndWaitForWal() waits for the *replay* LSN to increase, but gets woken up by walreceiver when new WAL has been flushed. Which means that typically walsenders will get woken up at the same time that the startup process will be - which means that by the time the logical walsender checks GetXLogReplayRecPtr() it's unlikely that the startup process already replayed the record and updated XLogCtl->lastReplayedEndRecPtr. Introducing a new condition variable to fix this corner case. --- src/backend/access/transam/xlogrecovery.c | 28 src/backend/replication/walsender.c | 31 +-- src/backend/utils/activity/wait_event.c | 3 +++ src/include/access/xlogrecovery.h | 3 +++ src/include/replication/walsender.h | 1 + src/include/utils/wait_event.h| 1 + 6 files changed, 59 insertions(+), 8 deletions(-) 41.2% src/backend/access/transam/ 48.5% src/backend/replication/ 3.6% src/backend/utils/activity/ 3.4% src/include/access/ diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index d5a81f9d83..ac8b169ab5 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -358,6 +358,9 @@ typedef struct XLogRecoveryCtlData RecoveryPauseState recoveryPauseState; ConditionVariable recoveryNotPausedCV; + /* Replay state (see getReplayedCV() for more explanation) */ + ConditionVariable replayedCV; + slock_t info_lck; /* locks shared variables shown above */ } XLogRecoveryCtlData; @@ -467,6 +470,7 @@ XLogRecoveryShmemInit(void) SpinLockInit(>info_lck); InitSharedLatch(>recoveryWakeupLatch); ConditionVariableInit(>recoveryNotPausedCV); + ConditionVariableInit(>replayedCV); } /* @@ -1916,6 +1920,11 @@ ApplyWalRecord(XLogReaderState *xlogreader, XLogRecord *record, TimeLineID *repl XLogRecoveryCtl->lastReplayedTLI = *replayTLI; SpinLockRelease(>info_lck); + /* +* wake up walsender(s) used by logical decoding on standby. +*/ + ConditionVariableBroadcast(>replayedCV); + /* * If rm_redo called XLogRequestWalReceiverReply, then we wake up the * receiver so that it notices the updated lastReplayedEndRecPtr and sends @@ -4916,3 +4925,22 @@ assign_recovery_target_xid(const char *newval, void *extra) else recoveryTarget = RECOVERY_TARGET_UNSET; } + +/* + * Return the ConditionVariable indicating that a replay has been done. + * + * This is needed for logical decoding on standby. Indeed the "problem" is that + * WalSndWaitForWal() waits for the *replay* LSN to
Re: Generate pg_stat_get_* functions with Macros
Hi, On 12/10/22 4:55 AM, Nathan Bossart wrote: On Fri, Dec 09, 2022 at 09:43:56PM -0500, Tom Lane wrote: Presumably it could be silenced by removing the semicolons after the new macro calls: The backslash after the last right brace means that the line following that is part of the macro body. This does no harm as long as said line is blank ... but I think it's a foot-gun waiting to bite somebody, because visually you'd think the macro ends with the brace. So I'd leave off that last backslash. Indeed. Patch attached. Oh right. Thanks Tom for the explanations and Nathan/Michael for the fix. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
