Re: Typo in ro.po file?
On 16.06.22 22:29, Bruce Momjian wrote: On Wed, Jun 15, 2022 at 09:29:02AM +0200, Peter Eisentraut wrote: On 14.06.22 05:34, Peter Smith wrote: FWIW, I stumbled on this obscure possible typo (?) in src/pl/plperl/po/ro.po: ~~~ #: plperl.c:788 msgid "while parsing Perl initialization" msgstr "în timpul parsing inițializării Perl" #: plperl.c:793 msgid "while running Perl initialization" msgstr "în timpul rulării intializării Perl" ~~~ (Notice the missing 'i' - "inițializării" versus "intializării") Fixed in translations repository. Thanks. What email list should such fixes be posted to? pgsql-translators@ would be ideal, but here is ok.
Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size
At Fri, 17 Jun 2022 15:59:26 +0900 (JST), Kyotaro Horiguchi wrote in > At Fri, 17 Jun 2022 15:54:13 +0900 (JST), Kyotaro Horiguchi > wrote in > > > Or we could add a timeout.c API that specifies the timeout? > > > > I sometimes wanted this, But I don't see a simple way to sort multiple > > relative timeouts in absolute time order. Maybe we can skip > > GetCurrentTimestamp only when inserting the first timeout, but I don't > > think it benefits this case. > > Or we can use a free-run interval timer and individual down-counter > for each timtouts. I think we need at-most 0.1s resolution and error > of long-run timer doesn't harm? Yeah, stupid. We don't want awake process with such a high frequency.. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: "buffer too small" or "path too long"?
On 15.06.22 19:08, Robert Haas wrote: On Wed, Jun 15, 2022 at 2:51 AM Peter Eisentraut wrote: We have this problem of long file names being silently truncated all over the source code. Instead of equipping each one of them with a length check, why don't we get rid of the fixed-size buffers and allocate dynamically, as in the attached patch. I've always wondered why we rely on MAXPGPATH instead of dynamic allocation. It seems pretty lame. I think it came in before we had extensible string buffers APIs.
Re: Extensible storage manager API - smgr hooks
Hello Yura and Anastasia.
I have tried to implement per-relation SMGR approach, and faced with a
serious problem with redo.
So, to implement per-relation SMGR feature i have tried to do things
similar to custom table AM apporach: that is, we can define our custom SMGR
in an extention (which defines smgr handle) and then use this SMGR in
relation definition. like this:
```postgres=# create extension proxy_smgr ;
CREATE EXTENSION
postgres=# select * from pg_smgr ;
oid | smgrname |smgrhandler
---++
4646 | md | smgr_md_handler
16386 | proxy_smgr | proxy_smgr_handler
(2 rows)
postgres=# create table tt(i int) storage manager proxy_smgr_handler;
ERROR: storage manager "proxy_smgr_handler" does not exist
postgres=# create table tt(i int) storage manager proxy_smgr;
INFO: proxy open 1663 5 16391
INFO: proxy create 16391
INFO: proxy close, 16391
INFO: proxy close, 16391
INFO: proxy close, 16391
INFO: proxy close, 16391
CREATE TABLE
postgres=# select * from tt;
INFO: proxy open 1663 5 16391
INFO: proxy nblocks 16391
INFO: proxy nblocks 16391
i
---
(0 rows)
postgres=# insert into tt values(1);
INFO: proxy exists 16391
INFO: proxy nblocks 16391
INFO: proxy nblocks 16391
INFO: proxcy extend 16391
INSERT 0 1
postgres=# select * from tt;
INFO: proxy nblocks 16391
INFO: proxy nblocks 16391
i
---
1
(1 row)
```
extention sql files looks like this:
```
CREATE FUNCTION proxy_smgr_handler(internal)
RETURNS table_smgr_handler
AS 'MODULE_PATHNAME'
LANGUAGE C;
-- Storage manager
CREATE STORAGE MANAGER proxy_smgr HANDLER proxy_smgr_handler;
```
To do this i have defined catalog relation pg_smgr where i store smgr`s
handlers and use this relation when we need to open some other(non-catalog)
relations in smgropen function. The patch almost passes regression tests(8
of 214 tests failed.) but it fails on first checkpoint or in crash
recorvery. Also, i have changed WAL format, added SMGR oid to each WAL
record with RelFileNode structure. Why do we need WAL changes? well, i
tried to solve folowing issue.
As i mentioned, there is a problem with redo, with is: we cannot do
syscache search to get relation`s SMGR to apply wal, because syscache is
not initialized during redo (crash recovery). As i understand, syscache is
not initialised because system catalogs are not consistent until crash
recovery is done.
So, thants it, I decided to write to this thread to get feedback and
understand how best to solve the problem with redo.
What do you think?
On Thu, Jun 16, 2022 at 1:38 PM Andres Freund wrote:
> Hi,
>
> On 2021-06-30 05:36:11 +0300, Yura Sokolov wrote:
> > Anastasia Lubennikova писал 2021-06-30 00:49:
> > > Hi, hackers!
> > >
> > > Many recently discussed features can make use of an extensible storage
> > > manager API. Namely, storage level compression and encryption [1],
> > > [2], [3], disk quota feature [4], SLRU storage changes [5], and any
> > > other features that may want to substitute PostgreSQL storage layer
> > > with their implementation (i.e. lazy_restore [6]).
> > >
> > > Attached is a proposal to change smgr API to make it extensible. The
> > > idea is to add a hook for plugins to get control in smgr and define
> > > custom storage managers. The patch replaces smgrsw[] array and smgr_sw
> > > selector with smgr() function that loads f_smgr implementation.
> > >
> > > As before it has only one implementation - smgr_md, which is wrapped
> > > into smgr_standard().
> > >
> > > To create custom implementation, a developer needs to implement smgr
> > > API functions
> > > static const struct f_smgr smgr_custom =
> > > {
> > > .smgr_init = custominit,
> > > ...
> > > }
> > >
> > > create a hook function
> > >
> > >const f_smgr * smgr_custom(BackendId backend, RelFileNode rnode)
> > > {
> > > //Here we can also add some logic and chose which smgr to use
> > > based on rnode and backend
> > > return &smgr_custom;
> > > }
> > >
> > > and finally set the hook:
> > > smgr_hook = smgr_custom;
> > >
> > > [1]
> > >
> https://www.postgresql.org/message-id/flat/11996861554042...@iva4-dd95b404a60b.qloud-c.yandex.net
> > > [2]
> > >
> https://www.postgresql.org/message-id/flat/272dd2d9.e52a.17235f2c050.Coremail.chjischj%40163.com
> > > [3] https://postgrespro.com/docs/enterprise/9.6/cfs
> > > [4]
> > >
> https://www.postgresql.org/message-id/flat/CAB0yre%3DRP_ho6Bq4cV23ELKxRcfhV2Yqrb1zHp0RfUPEWCnBRw%40mail.gmail.com
> > > [5]
> > >
> https://www.postgresql.org/message-id/flat/20180814213500.GA74618%4060f81dc409fc.ant.amazon.com
> > > [6]
> > > https://wiki.postgresql.org/wiki/PGCon_2021_Fun_With_WAL#Lazy_Restore
> > >
> > > --
> > >
> > > Best regards,
> > > Lubennikova Anastasia
> >
> > Good day, Anastasia.
> >
> > I also think smgr should be extended with different implementations
> aside of
> > md.
> > But which way concrete implementation will be chosen for particular
> > relation?
> > I believ
Re: Perform streaming logical transactions by background workers and parallel apply
On Fri, Jun 17, 2022 at 12:47 PM wangw.f...@fujitsu.com
wrote:
>
> Attach the new patches.
> Only changed patches 0001, 0004.
>
Few more comments on the previous version of patch:
===
1.
+/*
+ * Count the number of registered (not necessarily running) apply background
+ * worker for a subscription.
+ */
/worker/workers
2.
+static void
+apply_bgworker_setup_dsm(ApplyBgworkerState *wstate)
+{
...
...
+ int64 queue_size = 16000; /* 16 MB for now */
I think it would be better to use define for this rather than a
hard-coded value.
3.
+/*
+ * Status for apply background worker.
+ */
+typedef enum ApplyBgworkerStatus
+{
+ APPLY_BGWORKER_ATTACHED = 0,
+ APPLY_BGWORKER_READY,
+ APPLY_BGWORKER_BUSY,
+ APPLY_BGWORKER_FINISHED,
+ APPLY_BGWORKER_EXIT
+} ApplyBgworkerStatus;
It would be better if you can add comments to explain each of these states.
4.
+ /* Set up one message queue per worker, plus one. */
+ mq = shm_mq_create(shm_toc_allocate(toc, (Size) queue_size),
+(Size) queue_size);
+ shm_toc_insert(toc, APPLY_BGWORKER_KEY_MQ, mq);
+ shm_mq_set_sender(mq, MyProc);
I don't understand the meaning of 'plus one' in the above comment as
the patch seems to be setting up just one queue here?
5.
+
+ /* Attach the queues. */
+ wstate->mq_handle = shm_mq_attach(mq, seg, NULL);
Similar to above. If there is only one queue then the comment should
say queue instead of queues.
6.
snprintf(bgw.bgw_name, BGW_MAXLEN,
"logical replication worker for subscription %u", subid);
+ else
+ snprintf(bgw.bgw_name, BGW_MAXLEN,
+ "logical replication background apply worker for subscription %u ", subid);
No need for extra space after %u in the above code.
7.
+ launched = logicalrep_worker_launch(MyLogicalRepWorker->dbid,
+ MySubscription->oid,
+ MySubscription->name,
+ MyLogicalRepWorker->userid,
+ InvalidOid,
+ dsm_segment_handle(wstate->dsm_seg));
+
+ if (launched)
+ {
+ /* Wait for worker to attach. */
+ apply_bgworker_wait_for(wstate, APPLY_BGWORKER_ATTACHED);
In logicalrep_worker_launch(), we already seem to be waiting for
workers to attach via WaitForReplicationWorkerAttach(), so it is not
clear to me why we need to wait again? If there is a genuine reason
then it is better to add some comments to explain it. I think in some
way, we need to know if the worker is successfully attached and we may
not get that via WaitForReplicationWorkerAttach, so there needs to be
some way to know that but this doesn't sound like a very good idea. If
that understanding is correct then can we think of a better way?
8. I think we can simplify apply_bgworker_find_or_start by having
separate APIs for find and start. Most of the places need to use find
API except for the first stream. If we do that then I think you don't
need to make a hash entry unless we established ApplyBgworkerState
which currently looks odd as you need to remove the entry if we fail
to allocate the state.
9.
+ /*
+ * TO IMPROVE: Do we need to display the apply background worker's
+ * information in pg_stat_replication ?
+ */
+ UpdateWorkerStats(last_received, send_time, false);
In this do you mean to say pg_stat_subscription? If so, then to decide
whether we need to update stats here we should see what additional
information we can update here which is not possible via the main
apply worker?
10.
ApplyBgworkerMain
{
...
+ /* Load the subscription into persistent memory context. */
+ ApplyContext = AllocSetContextCreate(TopMemoryContext,
...
This comment seems to be copied from ApplyWorkerMain but doesn't apply here.
--
With Regards,
Amit Kapila.
Global variable/memory context for PostgreSQL functions
Dear PostgreSQL Developers, I'm currently working on a GiST extension (a new index structure) for PostgreSQL and I want to make it as customizable as I can. To achieve my goal I'm trying to take advantage of the options GiST support function to provide extra parameters to the operator class. Because I'm creating a new index structure, I've also developed new operators where I want to access the value of the operator class parameters as well. My main problem is that I can't do that, because the parameters are only accessible from the registered GiST support functions through specific macros. To solve the problem, I've tried to use global variables but it was very inconsistent because of the complex memory management of the whole system (also, I'm not as great in C programming as I want to be). Could you please help, by telling me that iss there any way to store and access values globally in PostgreSQL? I want to store these values in a way that is not affected by restarting the database server or maybe the whole computer. I would really appreciate your help. Thanks in advance! Best regards, Zsolt
Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size
On Thu, Jun 16, 2022 at 10:15 PM David Rowley wrote: > > On Fri, 17 Jun 2022 at 11:31, Andres Freund wrote: > > hashedscalararrayop/EEOP_HASHED_SCALARARRAYOP is 64 bytes, even though the > > limit is 40 bytes. > > > commit 50e17ad281b8d1c1b410c9833955bc80fbad4078 > > Author: David Rowley > > Date: 2021-04-08 23:51:22 +1200 > > > > Speedup ScalarArrayOpExpr evaluation > > I've put together the attached patch which removes 4 fields from the > hashedscalararrayop portion of the struct which, once the JSON part is > fixed, will put sizeof(ExprEvalStep) back down to 64 bytes again. > > The attached patch causes some extra pointer dereferencing to perform > a hashed saop step, so I tested the performance on f4fb45d15 (prior to > the JSON patch that pushed the sizeof(ExprEvalStep) up further. I > found: > > setup: > create table a (a int); > insert into a select x from generate_series(100,200) x; > > bench.sql > select * from a where a in(1,2,3,4,5,6,7,8,9,10); > > f4fb45d15 + reduce_sizeof_hashedsaop_ExprEvalStep.patch > drowley@amd3990x:~$ pgbench -n -f bench.sql -T 60 -M prepared postgres > tps = 44.841851 (without initial connection time) > tps = 44.986472 (without initial connection time) > tps = 44.944315 (without initial connection time) > > f4fb45d15 > drowley@amd3990x:~$ pgbench -n -f bench.sql -T 60 -M prepared postgres > tps = 44.446127 (without initial connection time) > tps = 44.614134 (without initial connection time) > tps = 44.895011 (without initial connection time) > > (Patched is ~0.61% faster here) > > So, there appears to be no performance regression due to the extra > indirection. There's maybe even some gains due to the smaller step > size. I didn't see that comment when working on this (it's quite a long unioned struct; I concur on adding an assert to catch it). This patch looks very reasonable to me though. James Coleman
Re: pg_upgrade (12->14) fails on aggregate
On Thu, Jun 16, 2022 at 10:01 PM Justin Pryzby wrote: > > Also, I'd be inclined to reject system-provided objects by checking > > for OID >= 16384 rather than hard-wiring assumptions about things > > being in pg_catalog or not. > > To me, oid>=16384 seems more hard-wired than namespace!='pg_catalog'. Extensions can be installed into pg_catalog, but they can't get low-numbered OIDs. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pg_upgrade (12->14) fails on aggregate
pá 17. 6. 2022 v 15:07 odesílatel Robert Haas napsal: > On Thu, Jun 16, 2022 at 10:01 PM Justin Pryzby > wrote: > > > Also, I'd be inclined to reject system-provided objects by checking > > > for OID >= 16384 rather than hard-wiring assumptions about things > > > being in pg_catalog or not. > > > > To me, oid>=16384 seems more hard-wired than namespace!='pg_catalog'. > > Extensions can be installed into pg_catalog, but they can't get > low-numbered OIDs. > yes Unfortunately, I did it in Orafce Regards Pavel > -- > Robert Haas > EDB: http://www.enterprisedb.com > > >
Re: pg_upgrade (12->14) fails on aggregate
Robert Haas writes: > On Thu, Jun 16, 2022 at 10:01 PM Justin Pryzby wrote: >> To me, oid>=16384 seems more hard-wired than namespace!='pg_catalog'. > Extensions can be installed into pg_catalog, but they can't get > low-numbered OIDs. Exactly. (To be clear, I had in mind writing something involving FirstNormalObjectId, not that you should put literal "16384" in the code.) regards, tom lane
Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size
Andres Freund writes: > The remaining difference looks like it's largely caused by the > enable_timeout_after(IDLE_STATS_UPDATE_TIMEOUT, ...) introduced as part of the > pgstats patch. It's only really visible when I pin a single connection pgbench > to the same CPU core as the server (which gives a ~16% boost here). > It's not the timeout itself - that we amortize nicely (via 09cf1d522). It's > that enable_timeout_after() does a GetCurrentTimestamp(). > Not sure yet what the best way to fix that is. Maybe not queue a new timeout if the old one is still active? BTW, it looks like that patch also falsified this comment (postgres.c:4478): * At most one of these timeouts will be active, so there's no need to * worry about combining the timeout.c calls into one. Maybe fixing that end of things would be a simpler way of buying back the delta. > Or we could add a timeout.c API that specifies the timeout? Don't think that will help: it'd be morally equivalent to enable_timeout_at(), which also has to do GetCurrentTimestamp(). regards, tom lane
Re: Pluggable toaster
Hi hackers, > For a pluggable toaster - in previous patch set part 7 patch file contains > invalid string. > Fixup (v2 file should used instead of previous) patch: > 7) 0007_fix_alignment_of_custom_toast_pointers.patch - fixes custom toast > pointer's > alignment required by bytea toaster by Nikita Glukhov; I finished digesting the thread and the referred presentations per Matthias (cc:'ed) suggestion in [1] discussion. Although the patchset got a fair amount of push-back above, I prefer to stay open minded and invest some of my time into this effort as a tester/reviewer during the July CF. Even if the patchset will not make it entirely to the core, some of its parts can be useful. Unfortunately, I didn't manage to find something that can be applied and tested. cfbot is currently not happy with the patchset. 0001_create_table_storage_v3.patch doesn't apply to the current origin/master manually either: ``` error: patch failed: src/backend/parser/gram.y:2318 error: src/backend/parser/gram.y: patch does not apply ``` Any chance we can see a rebased patchset for the July CF? [1]: https://commitfest.postgresql.org/38/3626/ -- Best regards, Aleksander Alekseev
Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size
I wrote: > Andres Freund writes: >> Or we could add a timeout.c API that specifies the timeout? > Don't think that will help: it'd be morally equivalent to > enable_timeout_at(), which also has to do GetCurrentTimestamp(). BTW, if we were willing to drop get_timeout_start_time(), it might be possible to avoid doing GetCurrentTimestamp() in enable_timeout_at, in the common case where the specified timestamp is beyond signal_due_at so that no setitimer call is needed. But getting the race conditions right could be tricky. On the whole this doesn't sound like something to tackle post-beta. regards, tom lane
Re: [PATCH] Compression dictionaries for JSONB
Hi Matthias, > These are just my initial thoughts I would like to share though. I may > change my mind after diving deeper into a "pluggable TOASTer" patch. I familiarized myself with the "pluggable TOASTer" thread and joined the discussion [1]. I'm afraid so far I failed to understand your suggestion to base "compression dictionaries" patch on "pluggable TOASTer", considering the fair amount of push-back it got from the community, not to mention a somewhat raw state of the patchset. It's true that Teodor and I are trying to address similar problems. This however doesn't mean that there should be a dependency between these patches. Also, I completely agree with Tomas [2]: > My main point is that we should not be making too many radical > changes at once - it makes it much harder to actually get anything done. IMO the patches don't depend on each other but rather complement each other. The user can switch between different TOAST methods, and the compression dictionaries can work on top of different TOAST methods. Although there is also a high-level idea (according to the presentations) to share common data between different TOASTed values, similarly to what compression dictionaries do, by looking at the current feedback and considering the overall complexity and the amount of open questions (e.g. interaction with different TableAMs, etc), I seriously doubt that this particular part of "pluggable TOASTer" will end-up in the core. [1]: https://postgr.es/m/CAJ7c6TOMPiRs-CZ%3DA9hyzxOyqHhKXxLD8qCF5%2BGJuLjQBzOX4A%40mail.gmail.com [2]: https://postgr.es/m/9ef14537-b33b-c63a-9938-e2b413db0a4c%40enterprisedb.com -- Best regards, Aleksander Alekseev
Re: Typo in ro.po file?
On Fri, Jun 17, 2022 at 09:01:42AM +0200, Peter Eisentraut wrote: > > > Fixed in translations repository. Thanks. > > > > What email list should such fixes be posted to? > > pgsql-translators@ would be ideal, but here is ok. Thanks. I see these posts occasionally and wanted to know where I should route them to, thanks. -- Bruce Momjian https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size
Hi, On 2022-06-17 14:14:54 +1200, David Rowley wrote: > I've put together the attached patch which removes 4 fields from the > hashedscalararrayop portion of the struct which, once the JSON part is > fixed, will put sizeof(ExprEvalStep) back down to 64 bytes again. > The attached patch causes some extra pointer dereferencing to perform > a hashed saop step, so I tested the performance on f4fb45d15 (prior to > the JSON patch that pushed the sizeof(ExprEvalStep) up further. I > found: What do you think about the approach prototyped in my patch to move the hash FunctionCallInfo into the element_tab? With a tiny bit more work that should reduce the amount of dereferincing over the state today, while also keeping below the limit? > setup: > create table a (a int); > insert into a select x from generate_series(100,200) x; > > bench.sql > select * from a where a in(1,2,3,4,5,6,7,8,9,10); > > f4fb45d15 + reduce_sizeof_hashedsaop_ExprEvalStep.patch > drowley@amd3990x:~$ pgbench -n -f bench.sql -T 60 -M prepared postgres > tps = 44.841851 (without initial connection time) > tps = 44.986472 (without initial connection time) > tps = 44.944315 (without initial connection time) > > f4fb45d15 > drowley@amd3990x:~$ pgbench -n -f bench.sql -T 60 -M prepared postgres > tps = 44.446127 (without initial connection time) > tps = 44.614134 (without initial connection time) > tps = 44.895011 (without initial connection time) > > (Patched is ~0.61% faster here) > > So, there appears to be no performance regression due to the extra > indirection. There's maybe even some gains due to the smaller step > size. "smaller step size"? Greetings, Andres Freund
Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size
Hi, On 2022-06-17 10:33:08 -0400, Tom Lane wrote: > Andres Freund writes: > > The remaining difference looks like it's largely caused by the > > enable_timeout_after(IDLE_STATS_UPDATE_TIMEOUT, ...) introduced as part of > > the > > pgstats patch. It's only really visible when I pin a single connection > > pgbench > > to the same CPU core as the server (which gives a ~16% boost here). > > > It's not the timeout itself - that we amortize nicely (via 09cf1d522). It's > > that enable_timeout_after() does a GetCurrentTimestamp(). > > > Not sure yet what the best way to fix that is. > > Maybe not queue a new timeout if the old one is still active? Right now we disable the timer after ReadCommand(). We can of course change that. At first I thought we might need more bookkeeping to do so, to avoid ProcessInterrupts() triggering pgstat_report_stat() when the timer fires later, but we probably can jury-rig something with DoingCommandRead && IsTransactionOrTransactionBlock() or such. I guess one advantage of something like this could be that we could possibly move the arming of the timeout to pgstat.c. But that looks like it might be more complicated than really worth it. > BTW, it looks like that patch also falsified this comment > (postgres.c:4478): > >* At most one of these timeouts will be active, so there's no > need to >* worry about combining the timeout.c calls into one. Hm, yea. I guess we can just disable them at once. > Maybe fixing that end of things would be a simpler way of buying back > the delta. I don't think that'll do the trick - in the case I'm looking at none of the other timers are active... > > Or we could add a timeout.c API that specifies the timeout? > > Don't think that will help: it'd be morally equivalent to > enable_timeout_at(), which also has to do GetCurrentTimestamp(). I should have been more precise - what I meant was a timeout.c API that allows the caller to pass in "now", which in this case we'd get from GetCurrentTransactionStopTimestamp(), which would avoid the additional timestamp computation. Greetings, Andres Freund
Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size
Andres Freund writes: > I should have been more precise - what I meant was a timeout.c API that allows > the caller to pass in "now", which in this case we'd get from > GetCurrentTransactionStopTimestamp(), which would avoid the additional > timestamp computation. I don't care for that one bit: it makes the accuracy of all timeouts dependent on how careful that caller is to provide an up-to-date "now". In the example at hand, there is WAY too much code between SetCurrentTransactionStopTimestamp() and the timer arming to make me think the results will be acceptable. regards, tom lane
Re: SGML doc file references
Peter Eisentraut wrote: > I think it was never a goal to absolutely make them match all the time, > so a lot of the differences might be accidental. ok, are they worth fixing? It seems like it'd make sense for files to properly reference other files so that humans don't have to go looking for files that don't exist... > There are also some tooling restrictions for what characters can be in the > output file names. I don't think that this applies to the changes I suggested in the patch I attached in my initial email.
Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size
Hi, On 2022-06-17 13:43:49 -0400, Tom Lane wrote: > Andres Freund writes: > > I should have been more precise - what I meant was a timeout.c API that > > allows > > the caller to pass in "now", which in this case we'd get from > > GetCurrentTransactionStopTimestamp(), which would avoid the additional > > timestamp computation. > > I don't care for that one bit: it makes the accuracy of all timeouts > dependent on how careful that caller is to provide an up-to-date "now". I don't think it'd necessarily have to influence the accuracy of all timeouts - but I've not looked at timeout.c much before. From what I understand we use 'now' for two things: First, to set ->start_time in enable_timeout() and second to schedule the alarm in schedule_alarm(). An inaccurate start_time won't cause problems for other timers afaics and it looks to me that it wouldn't be too hard to only require an accurate 'now' if the new timeout is nearest_timeout and now + nearest_timeout < signal_due_at? It's probably to complicated to tinker with now tho. Greetings, Andres Freund
Re: Checking for missing heap/index files
On 2022-Jun-09, Stephen Frost wrote: > TL;DR: if you're removing files from a directory that you've got an > active readdir() running through, you might not actually get all of the > *existing* files. Given that PG is happy to remove files from PGDATA > while a backup is running, in theory this could lead to a backup utility > like pgbackrest or pg_basebackup not actually backing up all the files. > > Now, pgbackrest runs the readdir() very quickly to build a manifest of > all of the files to backup, minimizing the window for this to possibly > happen, but pg_basebackup keeps a readdir() open during the entire > backup, making this more possible. Hmm, this sounds pretty bad, and I agree that a workaround should be put in place. But where is pg_basebackup looping around readdir()? I couldn't find it. There's a call to readdir() in FindStreamingStart(), but that doesn't seem to match what you describe. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Re: Using PQexecQuery in pipeline mode produces unexpected Close messages
On 2022-Jun-16, Kyotaro Horiguchi wrote:
> The attached is a crude patch to separate the state for PIPELINE-IDLE
> from PGASYNC_IDLE. I haven't found a better way..
Ah, yeah, this might be a way to fix this.
Something very similar to a PIPELINE_IDLE mode was present in Craig's
initial patch for pipeline mode. However, I fought very hard to remove
it, because it seemed to me that failing to handle it correctly
everywhere would lead to more bugs than not having it. (Indeed, there
were some.)
However, I see now that your patch would not only fix this bug, but also
let us remove the ugly "notionally not-idle" bit in fe-protocol3.c,
which makes me ecstatic. So let's push forward with this. However,
this means we'll have to go over all places that use asyncStatus to
ensure that they all handle the new value correctly.
I did found one bug in your patch: in the switch for asyncStatus in
PQsendQueryStart, you introduce a new error message. With the current
tests, that never fires, which is telling us that our coverage is not
complete. But with the right sequence of libpq calls, which the
attached adds (note that it's for REL_14_STABLE), that can be hit, and
it's easy to see that throwing an error there is a mistake. The right
action to take there is to let the action through.
Others to think about:
PQisBusy (I think no changes are needed),
PQfn (I think it should accept a call in PGASYNC_PIPELINE_IDLE mode;
fully untested in pipeline mode),
PQexitPipelineMode (I think it needs to return error; needs test case),
PQsendFlushRequest (I think it should let through; ditto).
I also attach a patch to make the test suite use Test::Differences, if
available. It makes the diffs of the traces much easier to read, when
they fail. (I wish for a simple way to set the context size, but that
would need a shim routine that I'm currently too lazy to write.)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
>From f96058efc70c3f72bc910308a9c2f64e4d3c7d8e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Wed, 15 Jun 2022 19:56:41 +0200
Subject: [PATCH v5 1/2] Avoid going IDLE in pipeline mode
Introduce a new PGASYNC_PIPELINE_IDLE state, which helps PQgetResult
distinguish the case of "really idle".
This fixes the problem that ReadyForQuery is sent too soon, which caused
a CloseComplete to be received when in idle state.
XXX -- this is still WIP.
Co-authored-by: Kyotaro Horiguchi
Reported-by: Daniele Varrazzo
Discussion: https://postgr.es/m/ca+mi_8bvd0_cw3sumgwpvwdnzxy32itog_16tdyru_1s2gv...@mail.gmail.com
---
src/interfaces/libpq/fe-connect.c | 1 +
src/interfaces/libpq/fe-exec.c| 45 +
src/interfaces/libpq/fe-protocol3.c | 12 ---
src/interfaces/libpq/libpq-int.h | 3 +-
.../modules/libpq_pipeline/libpq_pipeline.c | 99 +++
.../traces/simple_pipeline.trace | 37 +++
6 files changed, 166 insertions(+), 31 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 709ba15220..afd0bc809a 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -6751,6 +6751,7 @@ PQtransactionStatus(const PGconn *conn)
{
if (!conn || conn->status != CONNECTION_OK)
return PQTRANS_UNKNOWN;
+ /* XXX what should we do here if status is PGASYNC_PIPELINE_IDLE? */
if (conn->asyncStatus != PGASYNC_IDLE)
return PQTRANS_ACTIVE;
return conn->xactStatus;
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 4180683194..59f2e7f724 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -1279,7 +1279,8 @@ pqAppendCmdQueueEntry(PGconn *conn, PGcmdQueueEntry *entry)
* itself consume commands from the queue; if we're in any other
* state, we don't have to do anything.
*/
- if (conn->asyncStatus == PGASYNC_IDLE)
+ if (conn->asyncStatus == PGASYNC_IDLE ||
+conn->asyncStatus == PGASYNC_PIPELINE_IDLE)
{
resetPQExpBuffer(&conn->errorMessage);
pqPipelineProcessQueue(conn);
@@ -1642,9 +1643,9 @@ PQsendQueryStart(PGconn *conn, bool newQuery)
return false;
}
- /* Can't send while already busy, either, unless enqueuing for later */
- if (conn->asyncStatus != PGASYNC_IDLE &&
- conn->pipelineStatus == PQ_PIPELINE_OFF)
+ /* In non-pipeline mode, we can't send anything while already busy */
+ if (conn->pipelineStatus == PQ_PIPELINE_OFF &&
+ conn->asyncStatus != PGASYNC_IDLE)
{
appendPQExpBufferStr(&conn->errorMessage,
libpq_gettext("another command is already in progress\n"));
@@ -1667,11 +1668,13 @@ PQsendQueryStart(PGconn *conn, bool newQuery)
switch (conn->asyncStatus)
{
case PGASYNC_IDLE:
+ case PGASYNC_PIPELINE_IDLE:
case PGASYNC_READY:
case PGASYNC_READY_MORE:
case PGASYNC_BUSY:
/* ok to queue */
break;
+
case PGASYNC_COPY_IN:
case PGASYNC_COPY_OUT:
case PGASYNC_COPY_BOTH:
@@ -
Re: libpq: Remove redundant null pointer checks before free()
On 17.06.22 05:25, Michael Paquier wrote: On Thu, Jun 16, 2022 at 10:07:33PM +0200, Peter Eisentraut wrote: calls, where the "if" part is unnecessary. This is of course pretty harmless, but some functions like scram_free() and freePGconn() have become so bulky that it becomes annoying. So while I was doing some work in that area I undertook to simplify this. Seems fine. Would some of the buildfarm dinosaurs hiccup on that? gaur is one that comes into mind. I'm pretty sure PostgreSQL code already depends on this behavior anyway. The code just isn't consistent about it.
Re: libpq: Remove redundant null pointer checks before free()
On 17.06.22 07:11, Tom Lane wrote:
Having said that, the pattern "if (x) free(x);" is absolutely
ubiquitous across our code, and so I'm not sure that I'm on
board with undoing it only in libpq. I'd be happier if we made
a push to get rid of it everywhere.
Sure, here is a more comprehensive patch set. (It still looks like
libpq is the largest chunk.)
Notably, I think the choice
that pfree(NULL) is disallowed traces directly to worries about
coding-pattern-compatibility with pre-POSIX free(). Should we
revisit that?
Yes please, and also repalloc().From b8b24ade0f08077fe5a43a2373e86886f7e36abc Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Thu, 16 Jun 2022 21:50:56 +0200
Subject: [PATCH v2 1/3] Remove redundant null pointer checks before free()
---
.../pg_stat_statements/pg_stat_statements.c | 12 +-
contrib/uuid-ossp/uuid-ossp.c | 6 +-
src/backend/bootstrap/bootstrap.c | 3 +-
src/backend/libpq/auth.c | 5 +-
src/backend/postmaster/postmaster.c | 3 +-
src/backend/regex/regc_pg_locale.c| 6 +-
src/backend/tcop/postgres.c | 3 +-
src/backend/utils/adt/pg_locale.c | 30 +--
src/backend/utils/error/elog.c| 3 +-
src/backend/utils/init/miscinit.c | 3 +-
src/backend/utils/misc/guc.c | 24 +-
src/bin/pg_basebackup/pg_basebackup.c | 3 +-
src/bin/pg_basebackup/streamutil.c| 3 +-
src/bin/pg_dump/dumputils.c | 21 +-
src/bin/pg_dump/pg_backup_archiver.c | 60 ++---
src/bin/pg_dump/pg_backup_custom.c| 6 +-
src/bin/pg_dump/pg_backup_db.c| 3 +-
src/bin/pg_dump/pg_backup_tar.c | 3 +-
src/bin/pg_dump/pg_dump.c | 30 +--
src/bin/pg_dump/pg_dumpall.c | 6 +-
src/bin/pgbench/pgbench.c | 6 +-
src/bin/psql/command.c| 66 ++
src/bin/psql/copy.c | 3 +-
src/bin/psql/describe.c | 6 +-
src/bin/psql/input.c | 3 +-
src/bin/psql/tab-complete.c | 15 +-
src/common/fe_memutils.c | 3 +-
src/fe_utils/connect_utils.c | 3 +-
src/fe_utils/string_utils.c | 6 +-
src/interfaces/ecpg/pgtypeslib/numeric.c | 6 +-
src/interfaces/ecpg/preproc/descriptor.c | 3 +-
src/interfaces/libpq/fe-auth-scram.c | 33 +--
src/interfaces/libpq/fe-auth.c| 18 +-
src/interfaces/libpq/fe-connect.c | 211 ++
src/interfaces/libpq/fe-exec.c| 6 +-
src/interfaces/libpq/fe-print.c | 23 +-
src/interfaces/libpq/fe-secure-common.c | 3 +-
src/port/getaddrinfo.c| 3 +-
38 files changed, 214 insertions(+), 436 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c
b/contrib/pg_stat_statements/pg_stat_statements.c
index 768cedd91a..4acfddcdb8 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -809,8 +809,7 @@ pgss_shmem_shutdown(int code, Datum arg)
(errcode_for_file_access(),
errmsg("could not write file \"%s\": %m",
PGSS_DUMP_FILE ".tmp")));
- if (qbuffer)
- free(qbuffer);
+ free(qbuffer);
if (file)
FreeFile(file);
unlink(PGSS_DUMP_FILE ".tmp");
@@ -1657,8 +1656,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
pgss->extent != extent ||
pgss->gc_count != gc_count)
{
- if (qbuffer)
- free(qbuffer);
+ free(qbuffer);
qbuffer = qtext_load_file(&qbuffer_size);
}
}
@@ -1842,8 +1840,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
LWLockRelease(pgss->lock);
- if (qbuffer)
- free(qbuffer);
+ free(qbuffer);
}
/* Number of output arguments (columns) for pg_stat_statements_info */
@@ -2446,8 +2443,7 @@ gc_qtexts(void)
/* clean up resources */
if (qfile)
FreeFile(qfile);
- if (qbuffer)
- free(qbuffer);
+ free(qbuffer);
/*
* Since the contents of the external file are now uncertain, mark all
diff --git a/contrib/uuid-ossp/uuid-ossp.c b/contrib/uuid-ossp/uuid-ossp.c
index 7c0fb812fd..b868812358 100644
--- a/contrib/uuid-ossp/uuid-ossp.c
+++ b/contrib/uuid-ossp/uuid-ossp.c
@@ -292,8 +292,7 @@ uuid_generate_internal(int v, unsigned char *ns, const char
*ptr, int len)
if (ptr && len <= 36)
Re: SGML doc file references
On 17.06.22 19:52, Josh Soref wrote: Peter Eisentraut wrote: I think it was never a goal to absolutely make them match all the time, so a lot of the differences might be accidental. ok, are they worth fixing? That would require renaming either the output files or the input files, and people would really not like either one.
Re: libpq: Remove redundant null pointer checks before free()
Peter Eisentraut writes: > On 17.06.22 07:11, Tom Lane wrote: >> Notably, I think the choice >> that pfree(NULL) is disallowed traces directly to worries about >> coding-pattern-compatibility with pre-POSIX free(). Should we >> revisit that? > Yes please, and also repalloc(). repalloc no, because you wouldn't know which context to do the allocation in. regards, tom lane
Re: SGML doc file references
Peter Eisentraut writes: > On 17.06.22 19:52, Josh Soref wrote: >> ok, are they worth fixing? > That would require renaming either the output files or the input files, > and people would really not like either one. Agreed that renaming those files is not desirable, but the presented patch was only fixing erroneous/obsolete comments. regards, tom lane
Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size
Hi,
On 2022-06-17 10:30:55 -0700, Andres Freund wrote:
> On 2022-06-17 10:33:08 -0400, Tom Lane wrote:
> > Andres Freund writes:
> > > The remaining difference looks like it's largely caused by the
> > > enable_timeout_after(IDLE_STATS_UPDATE_TIMEOUT, ...) introduced as part
> > > of the
> > > pgstats patch. It's only really visible when I pin a single connection
> > > pgbench
> > > to the same CPU core as the server (which gives a ~16% boost here).
> >
> > > It's not the timeout itself - that we amortize nicely (via 09cf1d522).
> > > It's
> > > that enable_timeout_after() does a GetCurrentTimestamp().
> >
> > > Not sure yet what the best way to fix that is.
> >
> > Maybe not queue a new timeout if the old one is still active?
>
> Right now we disable the timer after ReadCommand(). We can of course change
> that. At first I thought we might need more bookkeeping to do so, to avoid
> ProcessInterrupts() triggering pgstat_report_stat() when the timer fires
> later, but we probably can jury-rig something with DoingCommandRead &&
> IsTransactionOrTransactionBlock() or such.
Here's a patch for that.
One thing I noticed is that disable_timeout() calls do
schedule_alarm(GetCurrentTimestamp()) if there's any other active timeout,
even if the to-be-disabled timer is already disabled. Of course callers of
disable_timeout() can guard against that using get_timeout_active(), but that
spreads repetitive code around...
I opted to add a fastpath for that, instead of using
get_timeout_active(). Afaics that's safe to do without disarming the signal
handler, but I'd welcome a look from somebody that knows this code.
> I guess one advantage of something like this could be that we could possibly
> move the arming of the timeout to pgstat.c. But that looks like it might be
> more complicated than really worth it.
I didn't do that yet, but am curious whether others think this would be
preferrable.
> > BTW, it looks like that patch also falsified this comment
> > (postgres.c:4478):
> >
> > * At most one of these timeouts will be active, so there's no
> > need to
> > * worry about combining the timeout.c calls into one.
>
> Hm, yea. I guess we can just disable them at once.
With the proposed change we don't need to change the separate timeout.c to
one, or update the comment, as it should now look the same as 14.
I also attached my heavily-WIP patches for the ExprEvalStep issues, I
accidentally had only included a small part of the contents of the json fix.
Greetings,
Andres Freund
>From 5fc220ce4ed8406e7c6852c6a6c59fb74a6e3f82 Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Fri, 17 Jun 2022 12:51:20 -0700
Subject: [PATCH v2 1/4] Add already-disabled fastpath to disable_timeout().
Otherwise callers that might call disable_timeout() frequently need to check
get_timeout_active() to avoid GetCurrentTimestamp() and other overheads in
disable_timeout().
Needed to avoid get_timeout_active() check in the subsequent commit fixing a
small performance regression.
---
src/backend/utils/misc/timeout.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.c
index 6f5e08bc302..d1219afa614 100644
--- a/src/backend/utils/misc/timeout.c
+++ b/src/backend/utils/misc/timeout.c
@@ -692,6 +692,11 @@ disable_timeout(TimeoutId id, bool keep_indicator)
Assert(all_timeouts_initialized);
Assert(all_timeouts[id].timeout_handler != NULL);
+ /* fast path for an already disabled timer */
+ if (!all_timeouts[id].active &&
+ (!all_timeouts[id].indicator || keep_indicator))
+ return;
+
/* Disable timeout interrupts for safety. */
disable_alarm();
--
2.35.1.677.gabf474a5dd
>From 41ff50e1264d2d32bc35b8b2c6c4ca375c90017a Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Fri, 17 Jun 2022 12:48:34 -0700
Subject: [PATCH v2 2/4] WIP: pgstat: reduce timer overhead.
While at it, also update assertion in pgstat_report_stat() to be more precise.
Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
src/backend/tcop/postgres.c | 47 ++---
src/backend/utils/activity/pgstat.c | 2 +-
2 files changed, 31 insertions(+), 18 deletions(-)
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 8b6b5bbaaab..a3aa9063dc9 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3371,10 +3371,13 @@ ProcessInterrupts(void)
IdleSessionTimeoutPending = false;
}
- if (IdleStatsUpdateTimeoutPending)
+ /*
+ * If there are pending stats updates and we currently are truly idle
+ * (matching the conditions in PostgresMain(), report stats now.
+ */
+ if (IdleStatsUpdateTimeoutPending &&
+ DoingCommandRead && !IsTransactionOrTransactionBlock())
{
- /* timer should have been disarmed */
- Assert(!IsTransactionBlock());
IdleStatsUpdateTimeoutPending = false;
pgstat_report_stat(true);
}
@@ -4051,7 +4054,6 @@ PostgresMain(const char *dbname,
Re: should check interrupts in BuildRelationExtStatistics ?
On Mon, Jun 06, 2022 at 04:23:34PM +0900, Michael Paquier wrote:
> On Sat, Jun 04, 2022 at 08:42:33PM -0500, Justin Pryzby wrote:
> > The fix seems to be to CHECK_FOR_INTERRUPTS() within multi_sort_compare().
> > That would supercede the other two CHECK_FOR_INTERRUPTS I'd proposed, and
> > handle mcv, depends, and ndistinct all at once.
>
> Hmm. I have to admit that adding a CFI() in multi_sort_compare()
> stresses me a bit as it is dependent on the number of rows involved,
> and it can be used as a qsort routine.
That's exactly the problem for which I showed a backtrace - it took 10s of
seconds to do qsort, which is (uh) a human timescale and too long to be
unresponsive, even if I create on a table with many rows a stats object with a
lot of columns and a high stats target.
>From 1c5fe1fb1cf4d2e38cf3b8edef1288cb63388cc4 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sun, 8 May 2022 19:36:43 -0500
Subject: [PATCH] wip: check for interrupts during extended stats
It's possible to handle this partially by adding CFI at higher levels
(statext_dependencies_build and statext_ndistinct_build), but in order to
handle MCV, CFI has to be done at a low level here.
---
src/backend/statistics/extended_stats.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index cb0a22b73e8..c6ba352e414 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -894,6 +894,8 @@ multi_sort_compare(const void *a, const void *b, void *arg)
SortItem *ib = (SortItem *) b;
int i;
+ CHECK_FOR_INTERRUPTS();
+
for (i = 0; i < mss->ndims; i++)
{
int compare;
--
2.17.1
Re: Checking for missing heap/index files
Greetings, On Fri, Jun 17, 2022 at 14:32 Alvaro Herrera wrote: > On 2022-Jun-09, Stephen Frost wrote: > > > TL;DR: if you're removing files from a directory that you've got an > > active readdir() running through, you might not actually get all of the > > *existing* files. Given that PG is happy to remove files from PGDATA > > while a backup is running, in theory this could lead to a backup utility > > like pgbackrest or pg_basebackup not actually backing up all the files. > > > > Now, pgbackrest runs the readdir() very quickly to build a manifest of > > all of the files to backup, minimizing the window for this to possibly > > happen, but pg_basebackup keeps a readdir() open during the entire > > backup, making this more possible. > > Hmm, this sounds pretty bad, and I agree that a workaround should be put > in place. But where is pg_basebackup looping around readdir()? I > couldn't find it. There's a call to readdir() in FindStreamingStart(), > but that doesn't seem to match what you describe. It’s the server side that does it in basebackup.c when it’s building the tarball for the data dir and each table space and sending it to the client. It’s not done by src/bin/pg_basebackup. Sorry for not being clear. Technically this would be beyond just pg_basebackup but would impact, potentially, anything using BASE_BACKUP from the replication protocol (in addition to other backup tools which operate against the data directory with readdir, of course). Thanks, Stephen >
Re: Add TAP test for auth_delay extension
2022년 6월 7일 (화) 오후 6:32, Dong Wook Lee 님이 작성: > > Hi Hackers, > I just wrote a test for `auth_delay` extension. > It's a test which confirms whether there is a delay for a second when > you enter the wrong password. > I sent an email using mutt, but I have a problem and sent it again. > > --- > Regards, > Dong Wook Lee. Hi, I have written a test for the auth_delay extension before, but if it is okay, can you review it? --- Regards, DongWook Lee.
Re: Add TAP test for auth_delay extension
On Sat, Jun 18, 2022 at 11:06:02AM +0900, Dong Wook Lee wrote: > I have written a test for the auth_delay extension before, > but if it is okay, can you review it? +# check enter wrong password +my $t0 = [gettimeofday]; +test_login($node, 'user_role', "wrongpass", 2); +my $elapsed = tv_interval($t0, [gettimeofday]); +ok($elapsed >= $delay_milliseconds / 1000, "auth_delay $elapsed seconds"); + +# check enter correct password +my $t0 = [gettimeofday]; +test_login($node, 'user_role', "pass", 0); +my $elapsed = tv_interval($t0, [gettimeofday]); +ok($elapsed < $delay_milliseconds / 1000, "auth_delay $elapsed seconds"); On a slow machine, I suspect that the second test is going to be unstable as it would fail if the login attempt (that succeeds) takes more than $delay_milliseconds. You could increase more delay_milliseconds to leverage that, but it would make the first test slower for nothing on faster machines in the case where the authentication attempt has failed. I guess that you could leverage that by using a large value for delay_milliseconds in the second test, because we are never going to wait. For the first test, you could on the contrary use a much lower value, still on slow machines it may not test what the code path of auth_delay you are willing to test. As a whole, I am not sure that this is really worth spending cycles on when running check-world or similar, and the code of the extension is trivial. -- Michael signature.asc Description: PGP signature
Re: libpq: Remove redundant null pointer checks before free()
On Fri, Jun 17, 2022 at 09:03:23PM +0200, Peter Eisentraut wrote: > I'm pretty sure PostgreSQL code already depends on this behavior anyway. > The code just isn't consistent about it. In the frontend, I'd like to think that you are right and that we have already some places doing that. The backend is a different story, like in GetMemoryChunkContext(). That should be easy enough to check with some LD_PRELOAD wizardry, at least. -- Michael signature.asc Description: PGP signature
Re: libpq: Remove redundant null pointer checks before free()
Michael Paquier writes: > On Fri, Jun 17, 2022 at 09:03:23PM +0200, Peter Eisentraut wrote: >> I'm pretty sure PostgreSQL code already depends on this behavior anyway. >> The code just isn't consistent about it. > In the frontend, I'd like to think that you are right and that we have > already some places doing that. We do, indeed. > The backend is a different story, > like in GetMemoryChunkContext(). That should be easy enough to check > with some LD_PRELOAD wizardry, at least. Huh? The proposal is to accept the fact that free() tolerates NULL, and then maybe make pfree() tolerate it as well. I don't think that that needs to encompass any other functions. regards, tom lane
