Composite types as parameters
Hello all,
I am trying to add support for composite types to my ORM, which uses libpq
and the binary format.
Given a schema like this one:
create type composite as (...);
create table sometable (field composite, ...);
I want to execute a query like this:
PQexecParams("insert into sometable values($1, ...);", paramValues[0] =
serialize some record, ...)
However this fails in coerce_record_to_complex(), because it receives a
node of type Param, but it can only handle RowExpr and Var. There is a
comment suggesting that this is not a fundamental limitation, but (not
being familiar with postgres codebase) I'm not sure how to go about fixing
it. I assume there is a mapping somewhere from param ids to objects, but
am unable to find it.
Does anyone have any pointers or suggestions? Am I going about this in
entirely the wrong way?
-E
Re: seawasp failing, maybe in glibc allocator
Seawasp should turn green on its next run. It did! -- Fabien.
Re: [PATCH] Make jsonapi usable from libpq
> On 26 Jun 2021, at 02:36, Michael Paquier wrote: > The service file parsing is the only piece in libpq using StringInfoData. > @Tom, @Daniel, you got involved in c0cb87f. It looks like this piece about > the > limitations with service file parsing needs a rework. This code is new in 14, > which means a new open item. Reworking it at this point to use a pqexpbuffer would be too invasive for 14 IMO, so reverting this part seems like the best option, and then redo it with a pqexpbuffer for 15. -- Daniel Gustafsson https://vmware.com/
Failover messages in Timeline History
Hi, if a failover (or probably a switchover, at least in the way Patroni does it) occurs, the timeline history (e.g. via "patronictl history"[1]) seems to read "no recovery target specified". That's correct, of course, from a PITR perspective, but for the (possibly more common?) promotion- of-a-standby-due-to-failover/switchover case rather misleading. I wonder whether it could be made more informative; like "no recovery target or failover" or "(standby) promotion witout recovery target"? Michael [1] root@pg1:~# patronictl -c /etc/patroni/13-main.yml history | head ++--+--+--+ | TL | LSN | Reason | Timestamp | ++--+--+--+ | 1 | 83886296 | no recovery target specified | 2021-06-18T20:04:11.645437+00:00 | | 2 | 83886928 | no recovery target specified | 2021-06-18T20:08:45.820304+00:00 | | 3 | 83887384 | no recovery target specified | 2021-06-19T05:57:50.431980+00:00 | | 4 | 83887840 | no recovery target specified | 2021-06-19T08:32:55.527975+00:00 | | 5 | 84017040 | no recovery target specified | 2021-06-19T12:05:40.495982+00:00 | | 6 | 84019264 | no recovery target specified | 2021-06-19T15:51:49.983987+00:00 | | 7 | 84135720 | no recovery target specified | 2021-06-20T03:46:22.775851+00:00 | -- Michael Banck Projektleiter / Senior Berater Tel.: +49 2166 9901-171 Fax: +49 2166 9901-100 Email: michael.ba...@credativ.de credativ GmbH, HRB Mönchengladbach 12080 USt-ID-Nummer: DE204566209 Trompeterallee 108, 41189 Mönchengladbach Geschäftsführung: Dr. Michael Meskes, Sascha Heuer Unser Umgang mit personenbezogenen Daten unterliegt folgenden Bestimmungen: https://www.credativ.de/datenschutz
Re: [HACKERS] WIP aPatch: Pgbench Serialization and deadlock errors
Hello Yugo-san, I'm wondering whether we could use "vars" instead of "variables" as a struct field name and function parameter name, so that is is shorter and more distinct from the type name "Variables". What do you think? The struct "Variables" has a field named "vars" which is an array of "Variable" type. I guess this is a reason why "variables" is used instead of "vars" as a name of "Variables" type variable so that we could know a variable's type is Variable or Variables. Also, in order to refer to the field, we would use vars->vars[vars->nvars] and there are nested "vars". Could this make a codereader confused? Hmmm… Probably. Let's keep "variables" then. -- Fabien.
Re: Doc chapter for Hash Indexes
On Fri, Jun 25, 2021 at 3:11 PM Simon Riggs wrote: > > On Fri, Jun 25, 2021 at 4:17 AM Amit Kapila wrote: > > > > On Fri, Jun 25, 2021 at 1:29 AM Bruce Momjian wrote: > > > > > > aOn Wed, Jun 23, 2021 at 12:56:51PM +0100, Simon Riggs wrote: > > > > On Wed, Jun 23, 2021 at 5:12 AM Amit Kapila > > > > wrote: > > > > > > > > > > On Tue, Jun 22, 2021 at 2:31 PM Simon Riggs > > > > > wrote: > > > > > > > > > > > > I attach both clean and compare versions. > > > > > > > > > > > > > > > > Do we want to hold this work for PG15 or commit in PG14 and backpatch > > > > > it till v10 where we have made hash indexes crash-safe? I would vote > > > > > for committing in PG14 and backpatch it till v10, however, I am fine > > > > > if we want to commit just to PG14 or PG15. > > > > > > > > Backpatch makes sense to me, but since not everyone will be reading > > > > this thread, I would look towards PG15 only first. We may yet pick up > > > > additional corrections or additions before a backpatch, if that is > > > > agreed. > > > > > > Yeah, I think backpatching makes sense. > > > > > > > I checked and found that there are two commits (7c75ef5715 and > > 22c5e73562) in the hash index code in PG-11 which might have impacted > > what we write in the documentation. However, AFAICS, nothing proposed > > in the patch would change due to those commits. Even, if we don't want > > to back patch, is there any harm in committing this to PG-14? > > I've reviewed those commits and the related code, so I agree. > Do you agree to just commit this to PG-14 or to commit in PG-14 and backpatch till PG-10? > As a result, I've tweaked the wording around VACUUM slightly. > Thanks, the changes look good to me. -- With Regards, Amit Kapila.
Re: subscription/t/010_truncate.pl failure on desmoxytes in REL_13_STABLE
On Fri, Jun 25, 2021 at 8:16 PM Tom Lane wrote: > > Amit Kapila writes: > > On Thu, Jun 24, 2021 at 11:25 PM Tom Lane wrote: > >> Checking the git history, this was fixed in f560209c6, which also > >> included some other mostly-cosmetic cleanup. I'm inclined to > >> propose back-patching that whole commit, rather than allowing the > >> code in exec_replication_command() to diverge in different branches. > >> It looks like it applies cleanly as far back as v10. Maybe something > >> could be done for 9.6 as well, but since that branch is so close to > >> EOL, I doubt it's worth spending extra effort on it. > > > +1. > > Done that way. > Thanks! -- With Regards, Amit Kapila.
Re: [HACKERS] WIP aPatch: Pgbench Serialization and deadlock errors
Hello Yugo-san, # About v12.2 ## Compilation Patch seems to apply cleanly with "git apply", but does not compile on my host: "undefined reference to `conditional_stack_reset'". However it works better when using the "patch". I'm wondering why git apply fails silently… When compiling there are warnings about "pg_log_fatal", which does not expect a FILE* on pgbench.c:4453. Remove the "stderr" argument. Global and local checks ok. number of transactions failed: 324 (3.240%) ... number of transactions retried: 5629 (56.290%) number of total retries: 103299 I'd suggest: "number of failed transactions". "total number of retries" or just "number of retries"? ## Feature The overall code structure changes to implements the feature seems reasonable to me, as we are at the 12th iteration of the patch. Comments below are somehow about details and asking questions about choices, and commenting… ## Documentation There is a lot of documentation, which is good. I'll review these separatly. It looks good, but having a native English speaker/writer would really help! Some output examples do not correspond to actual output for the current version. In particular, there is always one TPS figure given now, instead of the confusing two shown before. ## Comments transactinos -> transactions. ## Code By default max_tries = 0. Should not the initialization be 1, as the documentation argues that it is the default? Counter comments, missing + in the formula on the skipped line. Given that we manage errors, ISTM that we should not necessarily stop on other not retried errors, but rather count/report them and possibly proceed. Eg with something like: -- server side random fail DO LANGUAGE plpgsql $$ BEGIN IF RANDOM() < 0.1 THEN RAISE EXCEPTION 'unlucky!'; END IF; END; $$; Or: -- client side random fail BEGIN; \if random(1, 10) <= 1 SELECT 1 +; \else SELECT 2; \endif COMMIT; We could count the fail, rollback if necessary, and go on. What do you think? Maybe such behavior would deserve an option. --report-latencies -> --report-per-command: should we keep supporting the previous option? --failures-detailed: if we bother to run with handling failures, should it always be on? --debug-errors: I'm not sure we should want a special debug mode for that, I'd consider integrating it with the standard debug, or just for development. Also, should it use pg_log_debug? doRetry: I'd separate the 3 no retries options instead of mixing max_tries and timer_exceeeded, for clarity. Tries vs retries: I'm at odds with having tries & retries and + 1 here and there to handle that, which is a little bit confusing. I'm wondering whether we could only count "tries" and adjust to report what we want later? advanceConnectionState: ISTM that ERROR should logically be before others which lead to it. Variables management: it looks expensive, with copying and freeing variable arrays. I'm wondering whether we should think of something more clever. Well, that would be for some other patch. "Accumulate the retries" -> "Count (re)tries"? Currently, ISTM that the retry on error mode is implicitely always on. Do we want that? I'd say yes, but maybe people could disagree. ## Tests There are tests, good! I'm wondering whether something simpler could be devised to trigger serialization or deadlock errors, eg with a SEQUENCE and an \if. See the attached files for generating deadlocks reliably (start with 2 clients). What do you think? The PL/pgSQL minimal, it is really client-code oriented. Given that deadlocks are detected about every seconds, the test runs would take some time. Let it be for now. -- Fabien.
Re: Using each rel as both outer and inner for JOIN_ANTI
On Thu, Jun 24, 2021 at 10:14 PM Tom Lane wrote: > Heikki Linnakangas writes: > > On 24/06/2021 12:50, Richard Guo wrote: > >> I believe if we use the smaller table 'foo' as inner side for this > >> query, we would have a cheaper plan. > > > How would that work? > > I think you could make it work for the hash-join case by extending > the existing mechanism for right joins: emit nothing during the main > scan, but mark hashtable entries when a match is found. Then make > a post-pass and emit hash entries that never found a match. So > basically just the inverse behavior of a right join, but with the > same state info. > > Merge join could likely support "right anti join" too, though the > benefit of swapping inputs tends to be small there, so it may not be > worth doing. > > Obviously there's a pretty fair amount of code to write to make this > happen. > Thanks for the explanation. Attached is a demo code for the hash-join case, which is only for PoC to show how we can make it work. It's far from complete, at least we need to adjust the cost calculation for this 'right anti join'. Am I going in the right direction? Thanks Richard 0001-Using-each-rel-as-both-outer-and-inner-for-anti-join.patch Description: Binary data
Re: Composite types as parameters
Elijah Stone writes:
> I want to execute a query like this:
> PQexecParams("insert into sometable values($1, ...);", paramValues[0] =
> serialize some record, ...)
> However this fails in coerce_record_to_complex(), because it receives a
> node of type Param, but it can only handle RowExpr and Var.
You probably would have better results from specifying the composite
type explicitly in the query:
PQexecParams("insert into sometable values($1::composite, ...);",
I gather from the complaint that you're currently doing something that
causes the Param to be typed as a generic "record", which is problematic
since the record's details are not available from anyplace. But if you
cast it directly to a named composite type, that should work.
If it still doesn't work, please provide a more concrete example.
regards, tom lane
Re: PATCH: generate fractional cheapest paths in generate_orderedappend_path
Hi Tomas, I don't think there is much work left to do here. Did you have a look at the test case? Did it make sense to you? And I am sorry. I had another look at this and it seems I was confused (again). From: Arne Roland Sent: Monday, April 26, 2021 13:00 To: Tomas Vondra; pgsql-hackers Subject: Re: PATCH: generate fractional cheapest paths in generate_orderedappend_path > I think it should. We have a ParallelAppend node after all. > I'm not really familiar with the way > get_cheapest_fractional_path_for_pathkeys is used, but a quick search > suggests to > me, that build_minmax_path was thus far the only one using it. And minmax > paths are never parallel safe anyway. I think that is the reason it doesn't > do that already. The whole segment were are talking about obviously assumes require_parallel_safe is not needed. I wasn't aware that in set_append_rel_size. And I just realized there is a great comment explaining why it rightfully does so: /* * If any live child is not parallel-safe, treat the whole appendrel * as not parallel-safe. In future we might be able to generate plans * in which some children are farmed out to workers while others are * not; but we don't have that today, so it's a waste to consider * partial paths anywhere in the appendrel unless it's all safe. * (Child rels visited before this one will be unmarked in * set_append_rel_pathlist().) */ So afaik we don't need to think further about this. From: Tomas Vondra Sent: Thursday, June 3, 2021 22:57 To: Zhihong Yu Cc: Arne Roland; pgsql-hackers Subject: Re: PATCH: generate fractional cheapest paths in generate_orderedappend_path > Actually, there are two comments > >/* XXX maybe we should have startup_new_fractional? */ > > in the patch I posted - I completely forgot about that. But I think > that's a typo, I think - it should be > > /* XXX maybe we should have startup_neq_fractional? */ > > and the new flag would work similarly to startup_neq_total, i.e. it's > pointless to add paths where startup == fractional cost. > > At least I think that was the idea when I wrote the patch, it way too > long ago. > Sorry, I almost forgot about this myself. I only got reminded upon seeing > that again with different queries/tables. > Just to be sure I get this correctly: You mean startup_gt_fractional (cost) > as an additional condition, right? Could you clarify that for me? Regards Arne
Re: Rename of triggers for partitioned tables
Hello Álvaro Herrera, I am sorry to bother, but I am still annoyed by this. So I wonder if you had a look. Regards Arne From: Arne Roland Sent: Friday, April 2, 2021 7:55:16 PM To: Alvaro Herrera Cc: David Steele; Pg Hackers Subject: Re: Rename of triggers for partitioned tables Hi, attached a patch with some cleanup and a couple of test cases. The lookup is now done with the parentid and the renaming affects detached partitions as well. The test cases are not perfectly concise, but only add about 0.4 % to the total runtime of regression tests at my machine. So I didn't bother to much. They only consist of basic sql tests. Further feedback appreciated! Thank you! Regards Arne From: Arne Roland Sent: Thursday, April 1, 2021 4:38:59 PM To: Alvaro Herrera Cc: David Steele; Pg Hackers Subject: Re: Rename of triggers for partitioned tables Alvaro Herrera wrote: > I think the child cannot not have a corresponding trigger. If you try > to drop the trigger on child, it should say that it cannot be dropped > because the trigger on parent requires it. So I don't think there's any > case where altering name of trigger in parent would raise an user-facing > error. If you can't find the trigger in child, that's a case of catalog > corruption [...] Ah, I didn't know that. That changes my reasoning about that. I only knew, that the alter table ... detach partition ... doesn't detach the child trigger from the partitioned trigger, but the the attach partition seem to add one. Maybe we should be able to make sure that there is a one to one correspondence for child triggers and child partitions. Currently upon detaching we keep the trigger within the partitioned trigger of the parent relation. (So the trigger remains in place and can only be dropped with the parent one.) Only we try to attach the partition again any partitioned triggers are simply removed. One idea would be to detach partitioned triggers there in a similar manner we detach partitioned indexes. That could give above guarantee. (At least if one would be willing to write a migration for that.) But then we can't tell anymore where the trigger stems from. That hence would break our attach/detach workflow. I was annoyed by the inability to drop partitioned triggers from detached partitions, but it seems I just got a workaround. Ugh. This is a bit of a sidetrack discussion, but it is related. > Consider this. You have table p and partition p1. Add trigger t to p, > and do > ALTER TRIGGER t ON p1 RENAME TO q; > What I'm saying is that if you later do > ALTER TRIGGER t ON ONLY p RENAME TO r; > then the trigger on parent is changed, and the trigger on child stays q. > If you later do > ALTER TRIGGER r ON p RENAME TO s; > then triggers on both tables end up with name s. If we get into the mindset of expecting one trigger on the child, we have the following edge case: - The trigger is renamed. You seem to favor the silent rename in that case over raising a notice (or even an error). I am not convinced a notice wouldn't the better in that case. - The trigger is outside of partitioned table. That still means that the trigger might still be in the inheritance tree, but likely isn't. Should we rename it anyways, or skip that? Should be raise a notice about what we are doing here? I think that would be helpful to the end user. > Hmm, ok, maybe this is sufficient, I didn't actually test it. I think > you do need to add a few test cases to ensure everything is sane. Make > sure to verify what happens if you have multi-level partitioning > (grandparent, parent, child) and you ALTER the one in the middle. Also > things like if parent has two children and one child has multiple > children. The test I had somewhere upwards this thread, already tested that. I am not sure how to add a test for pg_dump though. Can you point me to the location in pg_regress for pg_dump? > Also, please make sure that it works correctly with INHERITS (legacy > inheritance). We probably don't want to cause recursion in that case. That works, but I will add a test to make sure it stays that way. Thank you for your input! Regards Arne
Re: Rename of triggers for partitioned tables
On 2021-Jun-26, Arne Roland wrote: > Hello Álvaro Herrera, > > I am sorry to bother, but I am still annoyed by this. So I wonder if you had > a look. I'll have a look during the commitfest which starts late next week. (However, I'm fairly sure that it won't be in released versions ... it'll only be fixed in the version to be released next year, postgres 15. Things that are clear bug fixes can be applied in released versions, but this patch probably changes behavior in ways that are not acceptable in released versions. Sorry about that.) -- Álvaro Herrera39°49'30"S 73°17'W Al principio era UNIX, y UNIX habló y dijo: "Hello world\n". No dijo "Hello New Jersey\n", ni "Hello USA\n".
Re: Rename of triggers for partitioned tables
From: Alvaro Herrera Sent: Saturday, June 26, 2021 18:36 Subject: Re: Rename of triggers for partitioned tables > I'll have a look during the commitfest which starts late next week. Thank you! > (However, I'm fairly sure that it won't be in released versions ... > it'll only be fixed in the version to be released next year, postgres > 15. Things that are clear bug fixes can be applied > in released versions, but this patch probably changes behavior in ways > that are not acceptable in released versions. Sorry about that.) I never expected any backports. Albeit I don't know anyone, who would mind, I agree with you on that assessment. This is very annoying, but not really breaking the product. Sure, I was hoping for pg14 initially. But I will survive another year of gexec. I am grateful you agreed to have a look! Regards Arne
Re: [PATCH] Make jsonapi usable from libpq
Michael Paquier writes:
> On Fri, Jun 25, 2021 at 08:58:46PM +, Jacob Champion wrote:
>> That surprised me. So there's currently no compiler-enforced
>> prohibition, just a policy? It looks like the bar was lowered a little
>> bit in commit c0cb87fbb6, as libpq currently has a symbol dependency on
>> pg_get_line_buf() and pfree() on my machine.
> Good point. That's worse than just pfree() which is just a plain call
> to free() in the frontend. We could have more policies here, but my
> take is that we'd better move fe_memutils.o to OBJS_FRONTEND in
> src/common/Makefile so as shared libraries don't use those routines in
> the long term.
Ugh. Not only is that bad, but your proposed fix doesn't fix it.
At least in psql, and probably in most/all of our other clients,
removing fe_memutils.o from libpq's link just causes it to start
relying on the copy in the psql executable :-(. So I agree that
some sort of mechanical enforcement would be a really good thing,
but I'm not sure what it would look like.
> In parseServiceFile(), initStringInfo() does a palloc() which would
> simply exit() on OOM, in libpq. That's not good. The service file
> parsing is the only piece in libpq using StringInfoData. @Tom,
> @Daniel, you got involved in c0cb87f.
I concur with Daniel that the easiest fix for v14 is to revert
c0cb87f. Allowing unlimited-length lines in the service file seems
like a nice-to-have, but it's not worth a lot. (Looking at the patch,
I'm inclined to keep much of the code rearrangement, just remove the
dependency on stringinfo.c. Also I'm tempted to set the fixed buffer
size at 1024 not 256, after which we might never need to improve it.)
I spent some time looking for other undesirable symbol dependencies
in libpq, and soon found a couple. PGTHREAD_ERROR potentially calls
abort(), which seems even worse than exit-on-OOM, although I don't
think we've ever heard a report of that being hit. Also,
fe-print.c's handling of OOM isn't nice at all:
fprintf(stderr, libpq_gettext("out of memory\n"));
abort();
Although fe-print.c is semi-deprecated, it still seems like it'd
be a good idea to clean that up.
BTW, so far as the original topic of this thread is concerned,
it sounds like Jacob's ultimate goal is to put some functionality
into libpq that requires JSON parsing. I'm going to say up front
that that sounds like a terrible idea. As we've just seen, libpq
operates under very tight constraints, not all of which are
mechanically enforced. I am really doubtful that anything that
would require a JSON parser has any business being in libpq.
Unless you can sell us on that point, I do not think it's worth
complicating the src/common JSON code to the point where it can
work under libpq's constraints.
regards, tom lane
Re: Rename of triggers for partitioned tables
On Sat, Jun 26, 2021 at 10:08 AM Arne Roland wrote: > *From:* Alvaro Herrera > *Sent:* Saturday, June 26, 2021 18:36 > *Subject:* Re: Rename of triggers for partitioned tables > > > I'll have a look during the commitfest which starts late next week. > > Thank you! > > > (However, I'm fairly sure that it won't be in released versions ... > > it'll only be fixed in the version to be released next year, postgres > > 15. Things that are clear bug fixes can be applied > > in released versions, but this patch probably changes behavior in ways > > that are not acceptable in released versions. Sorry about that.) > > I never expected any backports. Albeit I don't know anyone, who would > mind, I agree with you on that assessment. This is very annoying, but not > really breaking the product. > > Sure, I was hoping for pg14 initially. But I will survive another year of > gexec. > I am grateful you agreed to have a look! > > Regards > Arne > > Hi, Arne: It seems the patch no longer applies cleanly on master branch. Do you mind updating the patch ? Thanks 1 out of 7 hunks FAILED -- saving rejects to file src/backend/commands/trigger.c.rej patching file src/backend/parser/gram.y Hunk #1 succeeded at 8886 (offset 35 lines). patching file src/backend/utils/adt/ri_triggers.c Reversed (or previously applied) patch detected! Assume -R? [n] Apply anyway? [n] Skipping patch. 1 out of 1 hunk ignored -- saving rejects to file src/backend/utils/adt/ri_triggers.c.rej
Re: [PATCH] Make jsonapi usable from libpq
I wrote:
> I spent some time looking for other undesirable symbol dependencies
> in libpq, and soon found a couple. PGTHREAD_ERROR potentially calls
> abort(), which seems even worse than exit-on-OOM, although I don't
> think we've ever heard a report of that being hit. Also,
> fe-print.c's handling of OOM isn't nice at all:
> fprintf(stderr, libpq_gettext("out of memory\n"));
> abort();
> Although fe-print.c is semi-deprecated, it still seems like it'd
> be a good idea to clean that up.
fe-print.c seems easy enough to clean up, as per attached.
Not real sure what to do about PGTHREAD_ERROR.
regards, tom lane
diff --git a/src/interfaces/libpq/fe-print.c b/src/interfaces/libpq/fe-print.c
index fc7d84844e..0831950b12 100644
--- a/src/interfaces/libpq/fe-print.c
+++ b/src/interfaces/libpq/fe-print.c
@@ -37,7 +37,7 @@
#include "libpq-int.h"
-static void do_field(const PQprintOpt *po, const PGresult *res,
+static bool do_field(const PQprintOpt *po, const PGresult *res,
const int i, const int j, const int fs_len,
char **fields,
const int nFields, const char **fieldNames,
@@ -80,12 +80,12 @@ PQprint(FILE *fout, const PGresult *res, const PQprintOpt *po)
unsigned char *fieldNotNum = NULL;
char *border = NULL;
char **fields = NULL;
- const char **fieldNames;
+ const char **fieldNames = NULL;
int fieldMaxLen = 0;
int numFieldName;
int fs_len = strlen(po->fieldSep);
int total_line_length = 0;
- int usePipe = 0;
+ bool usePipe = false;
char *pagerenv;
#if defined(ENABLE_THREAD_SAFETY) && !defined(WIN32)
@@ -111,17 +111,17 @@ PQprint(FILE *fout, const PGresult *res, const PQprintOpt *po)
if (!(fieldNames = (const char **) calloc(nFields, sizeof(char *
{
fprintf(stderr, libpq_gettext("out of memory\n"));
- abort();
+ goto exit;
}
if (!(fieldNotNum = (unsigned char *) calloc(nFields, 1)))
{
fprintf(stderr, libpq_gettext("out of memory\n"));
- abort();
+ goto exit;
}
if (!(fieldMax = (int *) calloc(nFields, sizeof(int
{
fprintf(stderr, libpq_gettext("out of memory\n"));
- abort();
+ goto exit;
}
for (numFieldName = 0;
po->fieldName && po->fieldName[numFieldName];
@@ -190,7 +190,7 @@ PQprint(FILE *fout, const PGresult *res, const PQprintOpt *po)
fout = popen(pagerenv, "w");
if (fout)
{
- usePipe = 1;
+ usePipe = true;
#ifndef WIN32
#ifdef ENABLE_THREAD_SAFETY
if (pq_block_sigpipe(&osigset, &sigpipe_pending) == 0)
@@ -207,10 +207,11 @@ PQprint(FILE *fout, const PGresult *res, const PQprintOpt *po)
if (!po->expanded && (po->align || po->html3))
{
- if (!(fields = (char **) calloc(nFields * (nTups + 1), sizeof(char *
+ if (!(fields = (char **) calloc((size_t) nTups + 1,
+ nFields * sizeof(char *
{
fprintf(stderr, libpq_gettext("out of memory\n"));
-abort();
+goto exit;
}
}
else if (po->header && !po->html3)
@@ -264,9 +265,12 @@ PQprint(FILE *fout, const PGresult *res, const PQprintOpt *po)
fprintf(fout, libpq_gettext("-- RECORD %d --\n"), i);
}
for (j = 0; j < nFields; j++)
-do_field(po, res, i, j, fs_len, fields, nFields,
- fieldNames, fieldNotNum,
- fieldMax, fieldMaxLen, fout);
+ {
+if (!do_field(po, res, i, j, fs_len, fields, nFields,
+ fieldNames, fieldNotNum,
+ fieldMax, fieldMaxLen, fout))
+ goto exit;
+ }
if (po->html3 && po->expanded)
fputs("\n", fout);
}
@@ -297,18 +301,34 @@ PQprint(FILE *fout, const PGresult *res, const PQprintOpt *po)
for (i = 0; i < nTups; i++)
output_row(fout, po, nFields, fields,
fieldNotNum, fieldMax, border, i);
- free(fields);
- if (border)
-free(border);
}
if (po->header && !po->html3)
fprintf(fout, "(%d row%s)\n\n", PQntuples(res),
(PQntuples(res) == 1) ? "" : "s");
if (po->html3 && !po->expanded)
fputs("\n", fout);
- free(fieldMax);
- free(fieldNotNum);
- free((void *) fieldNames);
+
+exit:
+ if (fieldMax)
+ free(fieldMax);
+ if (fieldNotNum)
+ free(fieldNotNum);
+ if (border)
+ free(border);
+ if (fields)
+ {
+ /* if calloc succeeded, this shouldn't overflow size_t */
+ size_t numfields = ((size_t) nTups + 1) * (size_t) nFields;
+
+ while (numfields-- > 0)
+ {
+if (fields[numfields])
+ free(fields[numfields]);
+ }
+ free(fields);
+ }
+ if (fieldNames)
+ free((void *) fieldNames);
if (usePipe)
{
#ifdef WIN32
@@ -329,7 +349,7 @@ PQprint(FILE *fout, const PGresult *res, const PQprintOpt *po)
}
-static void
+static bool
do_field(const PQprintOpt *po, const PGresult *res,
const int i, const int j, const int fs_len,
char **fields,
@@ -397,7 +417,7 @@ do_field(const PQprintOpt *po, const PGresult *res,
if (!(fields[i * nFields + j] = (char *) malloc(plen + 1)))
{
fprintf(stderr, libpq_gettext("out of memory\n"));
-
Preventing abort() and exit() calls in libpq
[ starting a new thread so as not to confuse the cfbot ] I wrote: > Michael Paquier writes: >> Good point. That's worse than just pfree() which is just a plain call >> to free() in the frontend. We could have more policies here, but my >> take is that we'd better move fe_memutils.o to OBJS_FRONTEND in >> src/common/Makefile so as shared libraries don't use those routines in >> the long term. > Ugh. Not only is that bad, but your proposed fix doesn't fix it. > At least in psql, and probably in most/all of our other clients, > removing fe_memutils.o from libpq's link just causes it to start > relying on the copy in the psql executable :-(. So I agree that > some sort of mechanical enforcement would be a really good thing, > but I'm not sure what it would look like. After some thought I propose that what we really want is to prevent any calls of abort() or exit() from inside libpq. Attached is a draft patch to do that. This can't be committed as-is, because we still have some abort() calls in there in HEAD, but if we could get that cleaned up it'd work. Alternatively we could just disallow exit(), which'd be enough to catch the problematic src/common files. This relies on "nm" being able to work on shlibs, which it's not required to by POSIX. However, it seems to behave as desired even on my oldest dinosaurs. In any case, if "nm" doesn't work then we'll just not detect such problems on that platform, which should be OK as long as the test does work on common platforms. Other than that point I think it's relying only on POSIX-spec features. I'll stick this into the CF list to see if the cfbot agrees that it finds the abort() problems... regards, tom lane diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile index 0c4e55b6ad..3d992fdc78 100644 --- a/src/interfaces/libpq/Makefile +++ b/src/interfaces/libpq/Makefile @@ -96,12 +96,18 @@ SHLIB_EXPORTS = exports.txt PKG_CONFIG_REQUIRES_PRIVATE = libssl libcrypto -all: all-lib +all: all-lib check-libpq-refs # Shared library stuff include $(top_srcdir)/src/Makefile.shlib backend_src = $(top_srcdir)/src/backend +# Check for functions that libpq must not call. +# (If nm doesn't exist or doesn't work on shlibs, this test will silently +# do nothing, which is fine.) +.PHONY: check-libpq-refs +check-libpq-refs: $(shlib) + @! nm -A -g -u $< 2>/dev/null | grep -e abort -e exit # Make dependencies on pg_config_paths.h visible in all builds. fe-connect.o: fe-connect.c $(top_builddir)/src/port/pg_config_paths.h
Re: Pipeline mode and PQpipelineSync()
It hadn't occurred to me that I should ask the release management team about adding a new function to libpq this late in the cycle. Please do note that the message type used in the new routine is currenly unused and uncovered -- see line 4660 here: https://coverage.postgresql.org/src/backend/tcop/postgres.c.gcov.html -- Álvaro Herrera Valdivia, Chile "I'm impressed how quickly you are fixing this obscure issue. I came from MS SQL and it would be hard for me to put into words how much of a better job you all are doing on [PostgreSQL]." Steve Midgley, http://archives.postgresql.org/pgsql-sql/2008-08/msg0.php
Re: [PATCH] Make jsonapi usable from libpq
I wrote: > Not real sure what to do about PGTHREAD_ERROR. I wonder if we shouldn't simply nuke that macro and change the call sites into "Assert(false)". The only call sites are in default_threadlock() (used in fe_auth.c) and pq_lockingcallback() (for OpenSSL). I suggest that 1. "fprintf(stderr)" in these locking functions doesn't seem remarkably well-advised either. Especially not on Windows; but in general, we don't expect libpq to emit stuff on stderr except under *very* limited circumstances. 2. In an assert-enabled build, Assert() ought to be about equivalent to abort(). 3. In a production build, if one of these mutex calls fails, ignoring the failure might be the best thing to do anyway. Certainly, dumping core is the worst possible outcome, while not doing anything would have no impact except in the rather-unlikely case that multiple libpq connections try to use this code concurrently. It's certainly possible to quibble about point 3, but unless you have a better alternative to offer, I don't think you have a lot of room to complain. regards, tom lane
Re: Different compression methods for FPI
On Tue, Jun 22, 2021 at 12:53:46PM +0900, Michael Paquier wrote: > > So the patches that you say are unrelated still seem to me to be a > > prerequisite . > > I may be missing something, of course, but I still don't see why > that's necessary? We don't get any test failures on HEAD by switching > wal_compression to on, no? That's easy enough to test with a two-line > change that changes the default. Curious. I found that before a4d75c86bf, there was an issue without the "extra" patches. |commit a4d75c86bf15220df22de0a92c819ecef9db3849 |Author: Tomas Vondra |Date: Fri Mar 26 23:22:01 2021 +0100 | |Extended statistics on expressions I have no idea why that patch changes the behavior, but before a4d7, this patch series failed like: |$ time time make -C src/test/recovery check ... |# Failed test 'new xid after restart is greater' |# at t/011_crash_recovery.pl line 53. |# '539' |# > |# '539' | |# Failed test 'xid is aborted after crash' |# at t/011_crash_recovery.pl line 57. |# got: 'committed' |# expected: 'aborted' |# Looks like you failed 2 tests of 3. |t/011_crash_recovery.pl .. Dubious, test returned 2 (wstat 512, 0x200) |Failed 2/3 subtests I checked that my most recent WAL compression patch applied on top of a4d75c86bf works ok without the "extra" patches but fails when applied to a4d75c86bf~1. I think Andrey has been saying that since this already fails with PGLZ wal compression, we could consider this to be a pre-existing problem. I'm not thrilled with that interpretation, but it's not wrong. -- Justin
Re: unnesting multirange data types
On Mon, Jun 21, 2021 at 1:24 AM Alexander Korotkov wrote: > On Sun, Jun 20, 2021 at 11:09 AM Noah Misch wrote: > > On Sat, Jun 19, 2021 at 10:05:09PM -0400, Tom Lane wrote: > > > Alexander Korotkov writes: > > > > I also don't feel comfortable hurrying with unnest part to beta2. > > > > According to the open items wiki page, there should be beta3. Does > > > > unnest part have a chance for beta3? > > > > > > Hm. I'd prefer to avoid another forced initdb after beta2. On the > > > other hand, it's entirely likely that there will be some other thing > > > that forces that; in which case there'd be no reason not to push in > > > the unnest feature as well. > > > > > > I'd say let's sit on the unnest code for a little bit and see what > > > happens. > > > > I think $SUBJECT can't simultaneously offer too little to justify its own > > catversion bump and also offer enough to bypass feature freeze. If > > multirange > > is good without $SUBJECT, then $SUBJECT should wait for v15. Otherwise, the > > matter of the catversion bump should not delay commit of $SUBJECT. > > FWIW, there is a patch implementing just unnest() function. BTW, I found some small inconsistencies in the declaration of multirange operators in the system catalog. Nothing critical, but if we decide to bump catversion in beta3, this patch is also nice to push. -- Regards, Alexander Korotkov 0002-Fix-small-inconsistencies-in-catalog-definition-of--.patch Description: Binary data
Re: Pipeline mode and PQpipelineSync()
On Sat, Jun 26, 2021 at 05:40:15PM -0400, Alvaro Herrera wrote: > It hadn't occurred to me that I should ask the release management team > about adding a new function to libpq this late in the cycle. I have not followed the thread in details, but if you think that this improves the feature in the long term even for 14, I have no personally no objections to the addition of a new function, or even a change of behavior in one of the existing functions. The beta cycle is here for such adjustments. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Make jsonapi usable from libpq
On Sat, Jun 26, 2021 at 01:43:50PM -0400, Tom Lane wrote: > BTW, so far as the original topic of this thread is concerned, > it sounds like Jacob's ultimate goal is to put some functionality > into libpq that requires JSON parsing. I'm going to say up front > that that sounds like a terrible idea. As we've just seen, libpq > operates under very tight constraints, not all of which are > mechanically enforced. I am really doubtful that anything that > would require a JSON parser has any business being in libpq. > Unless you can sell us on that point, I do not think it's worth > complicating the src/common JSON code to the point where it can > work under libpq's constraints. AFAIK, the SASL mechanism OAUTHBEARER described in RFC 7628 would require such facilities as failures are reported in this format: https://datatracker.ietf.org/doc/html/rfc7628 Perhaps you are right and we have no need to do any json parsing in libpq as long as we pass down the JSON blob, but I am not completely sure if we can avoid that either. Separate topic: I find disturbing the dependency of jsonapi.c to the logging parts just to cope with dummy error values that are originally part of JsonParseErrorType. -- Michael signature.asc Description: PGP signature
Re: Composite types as parameters
On Sat, 26 Jun 2021, Tom Lane wrote:
You probably would have better results from specifying the composite
type explicitly in the query:
PQexecParams("insert into sometable values($1::composite, ...);",
I gather from the complaint that you're currently doing something that
causes the Param to be typed as a generic "record", which is problematic
since the record's details are not available from anyplace. But if you
cast it directly to a named composite type, that should work.
If it still doesn't work, please provide a more concrete example.
Thanks, unfortunately adding the explicit cast doesn't help. I've
attached a minimal runnable example.
I am serializing as a generic record, so it occurs to me that another
solution would be to use the actual type of the composite in question.
(Though it also seems to me that my code should work as-is.) Is there a
way to discover the OID of a composite type? And is the wire format the
same as for a generic record?
-E#include
#include
#include
#include
#define check(r) do { \
PGresult *res = r; \
if (PQresultStatus(res) == PGRES_NONFATAL_ERROR \
|| PQresultStatus(res) == PGRES_FATAL_ERROR) { \
puts(PQresultErrorMessage(res)); \
} \
} while (0)
int main() {
PGconn *c = PQconnectStart("postgresql://test:test@localhost/test");
assert(c);
for (int tries = 0; PQconnectPoll(c) != CONNECTION_MADE && tries < 10;
tries++)
usleep(10);
assert(PQconnectPoll(c) == CONNECTION_MADE);
check(PQexec(c, "CREATE TYPE some_record AS (x int);"));
check(PQexec(c, "CREATE TABLE tab (rec some_record, bar int);"));
// ok:
check(PQexec(c, "INSERT INTO tab VALUES(ROW(7), 8);"));
char recbuf[4096], *rec = recbuf;
*(int*)rec = __builtin_bswap32(1), rec += 4; //# elements
*(int*)rec = __builtin_bswap32(INT4OID), rec += 4;
*(int*)rec = __builtin_bswap32(4), rec += 4; //n bytes in entry
*(int*)rec = __builtin_bswap32(7), rec += 4;
// error:
check(PQexecParams(c, "INSERT INTO tab VALUES($1, 8);",
1, &(Oid){RECORDOID}, &(const char*){recbuf},
&(int){rec - recbuf}, &(int){1/*binary*/},
1/*binary result*/));
// error as well:
check(PQexecParams(c, "INSERT INTO tab VALUES($1::some_record, 8);",
1, &(Oid){RECORDOID}, &(const char*){recbuf},
&(int){rec - recbuf}, &(int){1},
1));
PQfinish(c);
}
Re: [HACKERS] Preserving param location
On Sat, Mar 11, 2017 at 11:09:32PM +0100, Julien Rouhaud wrote: > > When a query contains parameters, the original param node contains the token > location. However, this information is lost when the Const node is generated, > this one will only contain position -1 (unknown). > > FWIW, we do have a use case for this (custom extension that tracks quals > statistics, which among other thing is used to regenerate query string from a > pgss normalized query, thus needing the original param location). rebased v2.
Re: [HACKERS] Preserving param location
On Sun, Jun 27, 2021 at 11:31:53AM +0800, Julien Rouhaud wrote: > On Sat, Mar 11, 2017 at 11:09:32PM +0100, Julien Rouhaud wrote: > > > > When a query contains parameters, the original param node contains the token > > location. However, this information is lost when the Const node is > > generated, > > this one will only contain position -1 (unknown). > > > > FWIW, we do have a use case for this (custom extension that tracks quals > > statistics, which among other thing is used to regenerate query string from > > a > > pgss normalized query, thus needing the original param location). > > rebased v2. This time with the patch. >From 5c4831437e5b832d73dbab827d7f5e737bd1dfae Mon Sep 17 00:00:00 2001 From: Julien Rouhaud Date: Sat, 11 Mar 2017 22:48:00 +0100 Subject: [PATCH v2] Preserve param location when generating a const node. Discussion: https://postgr.es/m/20170311220932.GJ15188@nol.local --- src/backend/optimizer/util/clauses.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 059fa70254..084560acd8 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -2285,6 +2285,7 @@ eval_const_expressions_mutator(Node *node, int16 typLen; bool typByVal; Datum pval; + Const *con; get_typlenbyval(param->paramtype, &typLen, &typByVal); @@ -2292,13 +2293,17 @@ eval_const_expressions_mutator(Node *node, pval = prm->value; else pval = datumCopy(prm->value, typByVal, typLen); - return (Node *) makeConst(param->paramtype, + + con = makeConst(param->paramtype, param->paramtypmod, param->paramcollid, (int) typLen, pval, prm->isnull, typByVal); + con->location = param->location; + + return (Node *) con; } } } -- 2.31.1
Deparsing rewritten query
Hi,
I sometimes have to deal with queries referencing multiple and/or complex
views. In such cases, it's quite troublesome to figure out what is the query
really executed. Debug_print_rewritten isn't really useful for non trivial
queries, and manually doing the view expansion isn't great either.
While not being ideal, I wouldn't mind using a custom extension for that but
this isn't an option as get_query_def() is private and isn't likely to change.
As an alternative, maybe we could expose a simple SRF that would take care of
rewriting the query and deparsing the resulting query tree(s)?
I'm attaching a POC patch for that, adding a new pg_get_query_def(text) SRF.
Usage example:
SELECT pg_get_query_def('SELECT * FROM shoe') as def;
def
SELECT shoename, +
sh_avail, +
slcolor, +
slminlen, +
slminlen_cm, +
slmaxlen, +
slmaxlen_cm, +
slunit+
FROM ( SELECT sh.shoename, +
sh.sh_avail, +
sh.slcolor, +
sh.slminlen, +
(sh.slminlen * un.un_fact) AS slminlen_cm,+
sh.slmaxlen, +
(sh.slmaxlen * un.un_fact) AS slmaxlen_cm,+
sh.slunit +
FROM shoe_data sh, +
unit un +
WHERE (sh.slunit = un.un_name)) shoe; +
(1 row)
>From 5e726861fff28a276bb2e31972b278b833968ff4 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Sun, 27 Jun 2021 11:39:47 +0800
Subject: [PATCH v1] Add pg_get_query_def() to deparse and print a rewritten
SQL statement.
---
src/backend/rewrite/rewriteHandler.c | 2 +
src/backend/utils/adt/ruleutils.c| 70
src/include/catalog/pg_proc.dat | 3 ++
src/test/regress/expected/rules.out | 26 +++
src/test/regress/sql/rules.sql | 3 ++
5 files changed, 104 insertions(+)
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 88a9e95e33..f01b4531bf 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1815,6 +1815,8 @@ ApplyRetrieveRule(Query *parsetree,
rte->rtekind = RTE_SUBQUERY;
rte->subquery = rule_action;
rte->security_barrier = RelationIsSecurityView(relation);
+ if (!rte->alias)
+ rte->alias = makeAlias(get_rel_name(rte->relid), NULL);
/* Clear fields that should not be set in a subquery RTE */
rte->relid = InvalidOid;
rte->relkind = 0;
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 3719755a0d..cd39f4ce75 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -49,6 +49,8 @@
#include "nodes/nodeFuncs.h"
#include "nodes/pathnodes.h"
#include "optimizer/optimizer.h"
+#include "parser/analyze.h"
+#include "parser/parse_node.h"
#include "parser/parse_agg.h"
#include "parser/parse_func.h"
#include "parser/parse_node.h"
@@ -58,6 +60,7 @@
#include "rewrite/rewriteHandler.h"
#include "rewrite/rewriteManip.h"
#include "rewrite/rewriteSupport.h"
+#include "tcop/tcopprot.h"
#include "utils/array.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
@@ -489,6 +492,73 @@ static void get_reloptions(StringInfo buf, Datum reloptions);
#define only_marker(rte) ((rte)->inh ? "" : "ONLY ")
+/* return the query as postgres will rewrite */
+Datum
+pg_get_query_def(PG_FUNCTION_ARGS)
+{
+ char *sql = TextDatumGetCString(PG_GETARG_TEXT_PP(0));
+ List *parsetree_list;
+ List *querytree_list;
+ RawStmt *parsetree;
+ Query *query;
+ bool snapshot_set = false;
+ StringInfoData buf;
+ StringInfoData res;
+ ListCell *lc;
+
+ if (strcmp(sql, "") == 0)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("empty statement now allowed")));
+
+ parsetree_list = pg_parse_query(sql);
+
+ /* only support one statement at a time */
+ if (list_length(parsetree_list) != 1)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("a single statement should be provided")));
+
+ initStringInfo(&res);
+
+ parsetree = linitial_node(RawStmt, parsetree_list);
+
+ /*
+ * Set up a snapshot if parse analysis/planning will need one.
+ */
+ if (analyze_requires_snapshot(parsetree))
+ {
+ PushActiveSnapshot(GetTransactionSnapshot());
+ snapshot_set = true;
+ }
+
+ querytree_list = pg_analyze_and_rewrite(parsetree, sql,
+ NULL, 0, NULL);
+
+ /* Don
Re: Deparsing rewritten query
ne 27. 6. 2021 v 6:11 odesílatel Julien Rouhaud napsal:
> Hi,
>
> I sometimes have to deal with queries referencing multiple and/or complex
> views. In such cases, it's quite troublesome to figure out what is the
> query
> really executed. Debug_print_rewritten isn't really useful for non trivial
> queries, and manually doing the view expansion isn't great either.
>
> While not being ideal, I wouldn't mind using a custom extension for that
> but
> this isn't an option as get_query_def() is private and isn't likely to
> change.
>
> As an alternative, maybe we could expose a simple SRF that would take care
> of
> rewriting the query and deparsing the resulting query tree(s)?
>
> I'm attaching a POC patch for that, adding a new pg_get_query_def(text)
> SRF.
>
> Usage example:
>
> SELECT pg_get_query_def('SELECT * FROM shoe') as def;
> def
>
> SELECT shoename, +
> sh_avail, +
> slcolor, +
> slminlen, +
> slminlen_cm, +
> slmaxlen, +
> slmaxlen_cm, +
> slunit+
> FROM ( SELECT sh.shoename, +
> sh.sh_avail, +
> sh.slcolor, +
> sh.slminlen, +
> (sh.slminlen * un.un_fact) AS slminlen_cm,+
> sh.slmaxlen, +
> (sh.slmaxlen * un.un_fact) AS slmaxlen_cm,+
> sh.slunit +
> FROM shoe_data sh, +
> unit un +
>WHERE (sh.slunit = un.un_name)) shoe; +
>
> (1 row)
>
+1
Pavel
