Re: Incorrect usage of strtol, atoi for non-numeric junk inputs
On Sat, Jul 24, 2021 at 07:41:12PM +0900, Michael Paquier wrote:
> I have looked at that over the last couple of days, and applied it
> after some small fixes, including an indentation.
One thing that we forgot here is the handling of trailing
whitespaces. Attached is small patch to adjust that, with one
positive and one negative tests.
> The int64 and float
> parts are extra types we could handle, but I have not looked yet at
> how much benefits we'd get in those cases.
I have looked at these two but there is really less benefits, so for
now I am not planning to do more in this area. For float options,
pg_basebackup --max-rate could be one target on top of the three set
of options in pgbench, but it needs to handle units :(
--
Michael
From 2c38b36841965fc458dab69d846bcc0dde07aca2 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 26 Jul 2021 14:41:15 +0900
Subject: [PATCH] Skip trailing whitespaces when parsing integer options
strtoint(), via strtol(), would skip any leading whitespace but the same
rule was not applied for trailing whitespaces, which was a bit
inconsistent. Some tests are added while on it.
---
src/fe_utils/option_utils.c | 9 -
src/bin/pg_basebackup/t/020_pg_receivewal.pl | 4 +++-
src/bin/pg_dump/t/001_basic.pl | 3 ++-
3 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/fe_utils/option_utils.c b/src/fe_utils/option_utils.c
index 3e7e512ad9..bcfe7365fd 100644
--- a/src/fe_utils/option_utils.c
+++ b/src/fe_utils/option_utils.c
@@ -57,7 +57,14 @@ option_parse_int(const char *optarg, const char *optname,
errno = 0;
val = strtoint(optarg, &endptr, 10);
- if (*endptr)
+ /*
+ * Skip any trailing whitespace; if anything but whitespace remains before
+ * the terminating character, fail.
+ */
+ while (*endptr != '\0' && isspace((unsigned char) *endptr))
+ endptr++;
+
+ if (*endptr != '\0')
{
pg_log_error("invalid value \"%s\" for option %s",
optarg, optname);
diff --git a/src/bin/pg_basebackup/t/020_pg_receivewal.pl b/src/bin/pg_basebackup/t/020_pg_receivewal.pl
index 158f7d176f..47c4ecb073 100644
--- a/src/bin/pg_basebackup/t/020_pg_receivewal.pl
+++ b/src/bin/pg_basebackup/t/020_pg_receivewal.pl
@@ -88,10 +88,12 @@ SKIP:
$primary->psql('postgres',
'INSERT INTO test_table VALUES (generate_series(100,200));');
+ # Note the trailing whitespace after the value of --compress, that is
+ # a valid value.
$primary->command_ok(
[
'pg_receivewal', '-D', $stream_dir, '--verbose',
- '--endpos', $nextlsn, '--compress', '1'
+ '--endpos', $nextlsn, '--compress', '1 '
],
"streaming some WAL using ZLIB compression");
diff --git a/src/bin/pg_dump/t/001_basic.pl b/src/bin/pg_dump/t/001_basic.pl
index 59de6df7ba..d1a7e1db40 100644
--- a/src/bin/pg_dump/t/001_basic.pl
+++ b/src/bin/pg_dump/t/001_basic.pl
@@ -101,8 +101,9 @@ command_fails_like(
qr/\Qpg_dump: error: parallel backup only supported by the directory format\E/,
'pg_dump: parallel backup only supported by the directory format');
+# Note the trailing whitespace for the value of --jobs, that is valid.
command_fails_like(
- [ 'pg_dump', '-j', '-1' ],
+ [ 'pg_dump', '-j', '-1 ' ],
qr/\Qpg_dump: error: -j\/--jobs must be in range\E/,
'pg_dump: -j/--jobs must be in range');
--
2.32.0
signature.asc
Description: PGP signature
RE: [Doc] Tiny fix for regression tests example
On Monday, July 26, 2021 1:04 PM, Michael Paquier wrote: >> -make check PGOPTIONS="-c log_checkpoints=on -c work_mem=50MB" >> +make check PGOPTIONS="-c geqo=off -c work_mem=50MB" >> >> log_checkpoints couldn't be set in PGOPTIONS. >> >> Replace log_checkpoints with geqo in the example code >> >Right, that won't work. What about using something more >developer-oriented here, say force_parallel_mode=regress? Thanks for your comment. Agree with your suggestion. Modified it in the attachment patch. Regards, Tang V2-0001-minor-fix-for-regress-example.patch Description: V2-0001-minor-fix-for-regress-example.patch
Re: Added schema level support for publication.
On Mon, Jul 26, 2021 at 7:41 AM tanghy.f...@fujitsu.com < tanghy.f...@fujitsu.com> wrote: > > On Friday, July 23, 2021 8:18 PM vignesh C wrote: > > > > I have changed it to not report any error, this issue is fixed in the > > v14 patch attached at [1]. > > [1] - https://www.postgresql.org/message- > > id/CALDaNm3DTj535ezfmm8QHLOtOkcHF2ZcCfSjfR%3DVbTbLZXFRsA%40mail.g > > mail.com > > > > Thanks for your new patch. But there's a conflict when apply patch v14 on the latest HEAD (it seems caused by commit #678f5448c2d869), please rebase it. > Thanks for reporting it, it is rebased at the v15 patch attached at [1]. [1] - https://www.postgresql.org/message-id/CALDaNm27LRWF9ney%3DcVeD-0jc2%2BJ5Y0wNQhighZB%3DAat4VbNBA%40mail.gmail.com Regards, Vignesh
Re: [BUG]Update Toast data failure in logical replication
On Fri, Jul 23, 2021 at 8:58 AM Amit Kapila wrote: > > Okay, thanks. I think one point we need to consider here is that on > the subscriber side, we use dirtysnapshot to search the key, so we > need to ensure that we don't fetch the wrong data. I am not sure what > will happen when by the time we try to search the tuple in the > subscriber-side for the update, it has been removed and re-inserted > with the same values by the user. Do we find the newly inserted tuple > and update it? If so, can it also happen even if logged the unchanged > old_key_tuple as the patch is doing currently? > I was thinking more about this idea, but IMHO, unless we send the key toasted tuple from the publisher how is the subscriber supposed to fetch it. Because that is the key value for finding the tuple on the subscriber side and if we haven't sent the key value, how are we supposed to find the tuple on the subscriber side? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Inaccurate error message when set fdw batch_size to 0
On Thu, Jul 15, 2021 at 7:54 PM Bharath Rupireddy wrote: > > On Mon, Jul 12, 2021 at 10:11 PM Bharath Rupireddy > wrote: > > > > On Mon, Jul 12, 2021 at 9:20 PM Fujii Masao > > wrote: > > > + > > > + Avoid Using non-negative Word in Error > > > Messages > > > + > > > + > > > +Do not use non-negative word in error messages as it > > > looks > > > +ambiguous. Instead, use foo must be an integer value greater > > > than zero > > > +or foo must be an integer value greater than or equal to > > > zero > > > +if option foo expects an integer value. > > > + > > > + > > > > > > It seems suitable to put this guide under "Tricky Words to Avoid" > > > rather than adding it as separate section. Thought? > > > > +1. I will change. > > PSA v7 patch with the above change. PSA v8 patch rebased on to latest master. Regards, Bharath Rupireddy. v8-0001-Disambiguate-error-messages-that-use-non-negative.patch Description: Binary data
RE: logical replication empty transactions
On Friday, July 23, 2021 7:10 PM Ajin Cherian wrote: > On Fri, Jul 23, 2021 at 7:38 PM Peter Smith wrote: > > > > I have reviewed the v10 patch. The patch v11 looks good to me as well. Thanks for addressing my past comments. Best Regards, Takamichi Osumi
Re: [Doc] Tiny fix for regression tests example
On Fri, Jul 23, 2021 at 06:12:02AM +, tanghy.f...@fujitsu.com wrote: > Here's a tiny fix in regress.sgml. > > -make check PGOPTIONS="-c log_checkpoints=on -c work_mem=50MB" > +make check PGOPTIONS="-c geqo=off -c work_mem=50MB" > > log_checkpoints couldn't be set in PGOPTIONS. > > Replace log_checkpoints with geqo in the example code. Right, that won't work. What about using something more developer-oriented here, say force_parallel_mode=regress? > -make check EXTRA_REGRESS_OPTS="--temp-config=test_postgresql.conf" > +make check EXTRA_REGRESS_OPTS="--temp-config=$(pwd)/test_postgresql.conf" > > User needs to specify $(pwd) to let the command execute as expected. This works as-is. -- Michael signature.asc Description: PGP signature
Re: Planning counters in pg_stat_statements (using pgss_store)
On 2021/07/26 12:32, Julien Rouhaud wrote: On Sun, Jul 25, 2021 at 11:26:02PM -0400, Tom Lane wrote: Julien Rouhaud writes: I attach a patch that splits the test and add a comment explaining the boundaries for the new query. Checked with and without forced invalidations. Pushed with a little cosmetic fooling-about. Thanks! Thanks a lot! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Planning counters in pg_stat_statements (using pgss_store)
On Sun, Jul 25, 2021 at 11:26:02PM -0400, Tom Lane wrote: > Julien Rouhaud writes: > > I attach a patch that splits the test and add a comment explaining the > > boundaries for the new query. > > Checked with and without forced invalidations. > > Pushed with a little cosmetic fooling-about. Thanks!
Re: Planning counters in pg_stat_statements (using pgss_store)
Julien Rouhaud writes: > I attach a patch that splits the test and add a comment explaining the > boundaries for the new query. > Checked with and without forced invalidations. Pushed with a little cosmetic fooling-about. regards, tom lane
Re: Outdated comment in get_agg_clause_costs
On Fri, 23 Jul 2021 at 02:29, David Rowley wrote: > I've attached a patch that adjusts the comment so it's more aligned to > what it now does. This was a bit more outdated than I first thought. I also removed the mention of the function setting the aggtranstype and what it mentions about also gathering up "counts". I assume that was related to numOrderedAggs which is now done in preprocess_aggref(). I ended up pushing to master and PG14. The code was new to PG14 so I thought it made sense to keep master and 14 the same since 14 is not yet out the door. David
Re: Added schema level support for publication.
On Fri, Jul 23, 2021 at 10:16 PM vignesh C wrote: > > I have changed it to pg_pubication_schema as earlier as both you and Houzj > San felt pg_publication_schema was better. This is fixed in the v14 patch > attached at [1]. > [1] - > https://www.postgresql.org/message-id/CALDaNm3DTj535ezfmm8QHLOtOkcHF2ZcCfSjfR%3DVbTbLZXFRsA%40mail.gmail.com > Thanks, I think it looks better. On another issue, there seems to be problems with pg_dump support for FOR SCHEMA publications. For example: CREATE PUBLICATION p FOR SCHEMA myschema; pg_dump is dumping: CREATE PUBLICATION p WITH (publish = 'insert, update, delete, truncate'); ... ALTER PUBLICATION p ADD SCHEMA p; Obviously that last line should instead be "ALTER PUBLICATION p ADD SCHEMA myschema;" I think the bug is caused by the following: + appendPQExpBuffer(query, + "ALTER PUBLICATION %s ADD SCHEMA %s;\n", + fmtId(pubsinfo->pubname), fmtId(schemainfo->dobj.name)); The comment for fmtId() says: * Note that the returned string must be used before calling fmtId again, * since we re-use the same return buffer each time. So I think there was a reason why the ALTER PUBLICATION and ADD SCHEMA were previously formatted separately (and so should NOT have been combined). It should be restored to how it was in the previous patch version. Regards, Greg Nancarrow Fujitsu Australia
Re: Removing "long int"-related limit on hash table sizes
On Sun, Jul 25, 2021 at 12:28:04PM -0400, Tom Lane wrote: > Andres Freund writes: >> We really ought to just remove every single use of long. > > I have no objection to that as a long-term goal. But I'm not volunteering > to do all the work, and in any case it wouldn't be a back-patchable fix. > I feel that we do need to do something about this performance regression > in v13. Another idea may be to be more aggressive in c.h? A tweak there would be dirtier than marking long as deprecated, but that would be less invasive. Any of that is not backpatchable, of course.. > No, I don't like that. Using size_t for memory-size variables is good > discipline. Moreover, I'm not convinced that even with 64-bit ints, > overflow would be impossible in all the places I fixed here. They're > multiplying several potentially very large values (one of which > is a float). I think this is just plain sloppy coding, independently > of which bit-width you choose to be sloppy in. Yeah, using size_t where adapted is usually a good idea. -- Michael signature.asc Description: PGP signature
Re: Fix memory leak when output postgres_fdw's "Relations"
On Fri, Jul 23, 2021 at 04:20:37PM -0300, Ranier Vilela wrote: > Maybe not yet. Valgrind may also don't understand yet. I think that you should do things the opposite way. In short, instead of attempting to understand first Valgrind or Coverity and then Postgres, try to understand the internals of Postgres first and then interpret what Valgrind or even Coverity tell you. Tom is right. There is no point in caring about the addition some pfree()'s in the backend code as long as they don't prove to be an actual leak in the context where a code path is used, and nobody will follow you on that. Some examples where this would be worth caring about are things like tight loops leaking a bit of memory each time these are taken, with leaks that can be easily triggered by the user with some specific SQL commands, or even memory contexts not cleaned up where they should, impacting some parts of the system (like the executor, or even the planner) for a long-running analytical query. -- Michael signature.asc Description: PGP signature
RE: Added schema level support for publication.
On Friday, July 23, 2021 8:18 PM vignesh C wrote: > > I have changed it to not report any error, this issue is fixed in the > v14 patch attached at [1]. > [1] - https://www.postgresql.org/message- > id/CALDaNm3DTj535ezfmm8QHLOtOkcHF2ZcCfSjfR%3DVbTbLZXFRsA%40mail.g > mail.com > Thanks for your new patch. But there's a conflict when apply patch v14 on the latest HEAD (it seems caused by commit #678f5448c2d869), please rebase it. Besides, I tested your patch on old HEAD and confirmed that the issue I reported was fixed. Regards Tang
Re: logical replication empty transactions
On Fri, Jul 23, 2021 at 8:09 PM Ajin Cherian wrote: > > fixed. The v11 patch LGTM. Regards, Greg Nancarrow Fujitsu Australia
Re: Planning counters in pg_stat_statements (using pgss_store)
On Mon, Jul 26, 2021 at 01:08:08AM +0800, Julien Rouhaud wrote: > Le lun. 26 juil. 2021 à 00:59, Tom Lane a écrit : > > > Julien Rouhaud writes: > > > On Sun, Jul 25, 2021 at 12:03:25PM -0400, Tom Lane wrote: > > > > > > Would it be worth to split the query for the prepared statement row vs > > the rest > > > to keep the full "plans" coverage when possible? > > > > +1, the same thought occurred to me later. Also, if we're making > > it specific to the one PREPARE example, we could get away with > > checking "plans >= 2 AND plans <= calls", with a comment like > > "we expect at least one replan event, but there could be more". > > > > Do you want to prepare a patch? > > > > Sure, I will work on that tomorrow! I attach a patch that splits the test and add a comment explaining the boundaries for the new query. Checked with and without forced invalidations. >From 225632c54ff734f7f2b5a2b0d45127e425327973 Mon Sep 17 00:00:00 2001 From: Julien Rouhaud Date: Mon, 26 Jul 2021 09:23:57 +0800 Subject: [PATCH v1] Make pg_stat_statements tests immune to prepared statements invalidation. The tests for the number of planifications have been unstable since their introduction, but got unnoticed until now as buildfarm animals don't run them for now. Discussion: https://postgr.es/m/42557.1627229...@sss.pgh.pa.us --- .../expected/pg_stat_statements.out | 27 --- .../sql/pg_stat_statements.sql| 5 +++- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index 40b5109b55..ea2f8cf77f 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -850,16 +850,23 @@ SELECT 42; 42 (1 row) -SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C"; -query| plans | calls | rows --+---+---+-- - ALTER TABLE test ADD COLUMN x int | 0 | 1 |0 - CREATE TABLE test ()| 0 | 1 |0 - PREPARE prep1 AS SELECT COUNT(*) FROM test | 2 | 4 |4 - SELECT $1 | 3 | 3 |3 - SELECT pg_stat_statements_reset() | 0 | 1 |1 - SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C" | 1 | 0 |0 -(6 rows) +SELECT query, plans, calls, rows FROM pg_stat_statements WHERE query NOT LIKE 'PREPARE%' ORDER BY query COLLATE "C"; +query| plans | calls | rows +-+---+---+-- + ALTER TABLE test ADD COLUMN x int | 0 | 1 |0 + CREATE TABLE test ()| 0 | 1 |0 + SELECT $1 | 3 | 3 |3 + SELECT pg_stat_statements_reset() | 0 | 1 |1 + SELECT query, plans, calls, rows FROM pg_stat_statements WHERE query NOT LIKE $1 ORDER BY query COLLATE "C" | 1 | 0 |0 +(5 rows) + +-- for the prepared statemennt we expect at least one replan, but cache +-- invalidations could force more +SELECT query, plans >=2 AND plans<=calls AS plans_ok, rows FROM pg_stat_statements WHERE query LIKE 'PREPARE%' ORDER BY query COLLATE "C"; + query| plans_ok | rows ++--+-- + PREPARE prep1 AS SELECT COUNT(*) FROM test | t|4 +(1 row) -- -- access to pg_stat_statements_info view diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sql index bc3b6493e6..3606a5f581 100644 --- a/contrib/pg_stat_statements/sql/pg_stat_statements.sql +++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql @@ -356,7 +356,10 @@ EXECUTE prep1; SELECT 42; SELECT 42; SELECT 42; -SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C"; +SELECT query, plans, calls, rows FROM pg_stat_statements WHERE query NOT LIKE 'PREPARE%' ORDER BY query COLLATE "C"; +-- for the prepared statemennt we expect at
Re: logical replication empty transactions
FYI - I have checked the v11 patch. Everything applies, builds, and tests OK for me, and I have no more review comments. So v11 LGTM. -- Kind Regards, Peter Smith. Fujitsu Australia
Re: Planning counters in pg_stat_statements (using pgss_store)
Andrew Dunstan writes:
> On 7/25/21 12:03 PM, Tom Lane wrote:
>> So AFAICS this test is inherently unstable and there is no code bug
>> to be fixed. We could drop the "plans" column from this query, or
>> print something approximate like "plans > 0 AND plans <= calls".
>> Thoughts?
> Is that likely to tell us anything very useful?
The variant suggested downthread ("plans >= 2 AND plans <= calls" for the
PREPARE entry only) seems like it's still reasonably useful. At least it
can verify that a replan has occurred and been counted.
regards, tom lane
Re: Planning counters in pg_stat_statements (using pgss_store)
On 7/25/21 12:03 PM, Tom Lane wrote: > > So AFAICS this test is inherently unstable and there is no code bug > to be fixed. We could drop the "plans" column from this query, or > print something approximate like "plans > 0 AND plans <= calls". > Thoughts? > Is that likely to tell us anything very useful? I suppose it's really just a check against insane values. Since the test is unstable it's hard to do more than that. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Removing "long int"-related limit on hash table sizes
Em dom., 25 de jul. de 2021 às 15:53, Tom Lane escreveu: > Ranier Vilela writes: > > I think int64 is in most cases the counterpart of *long* on Windows. > > I'm not particularly on board with s/long/int64/g as a universal > solution. Sure, not a universal solution, I mean a start point. When I look for a type that is signed and size 8 bytes in Windows, I only see int64. I think that most of these usages are concerned with > memory sizes and would be better off as "size_t". Ok, but let's not forget that size_t is unsigned. We might need > int64 in places where we're concerned with sums of memory usage > across processes, or where the value needs to be allowed to be > negative. So it'll take case-by-case analysis to do it right. > Sure. > BTW, one aspect of this that I'm unsure how to tackle is the > common usage of "L" constants; in particular, "work_mem * 1024L" > is a really common idiom that we'll need to get rid of. Not sure > that grep will be a useful aid for finding those. > I can see 30 matches in the head tree. (grep -d "1024L" *.c) File backend\access\gin\ginfast.c: if (metadata->nPendingPages * GIN_PAGE_FREESIZE > cleanupSize * 1024L) (accum.allocatedMemory >= workMemory * 1024L))) Is it a good point to start? or one more simple? (src/backend/access/hash/hash.c) has one *long*. regards, Ranier Vilela
Re: Have I found an interval arithmetic bug?
> On 23-Jul-2021, br...@momjian.us wrote: > > On Fri, Jul 23, 2021 at 10:55:11AM -0700, Bryn Llewellyn wrote: >> SELECT >> '1.2345 months 1.2345 days 1.2345 seconds'::interval = >> '1 month 1 day 1 second'::interval*1.2345; >> >> In 13.3, the result is TRUE. (I know that this doesn’t guarantee that the >> internal representations of the two compared interval values are the same. >> But >> it’s a necessary condition for the outcome that I’m referring to and serves >> to >> indecate the pont I’m making. A more careful test can be made. > > So you are saying fractional unit output should match multiplication > output? It doesn't now for all units: > > SELECT interval '1.3443 years'; > interval > --- >1 year 4 mons > > SELECT interval '1 years' * 1.3443; > ?column? > - >1 year 4 mons 3 days 22:45:07.2 > > It is true this patch is further reducing that matching. Do people > think I should make them match as part of this patch? Summary: It seems to me that the best thing to do is fix the coarse bug about which there is unanimous agreement and to leave everything else (quirks and all) untouched. Detail: --- My previous email (to which Bruce replies here) was muddled. Sorry. The challenge is that are a number of mechanisms at work. Their effects are conflated. And it’s very hard to unscramble them. The underlying problem is that the internal representation of an interval value is a [mm, dd, ss] tuple. This fact is documented. The representation is key to understanding the definitions of these operations: — defining an interval value from a text literal that uses real number values for its various fields. — defining an interval value from make_interval(). (This one is easy because the API requires integral values for all but the seconds argument. It would be interesting to know why this asymmetrical definition was implemented. It seems to imply that somebody though that spilldown was a bad idea and should be prevented before it might happen.) — creating the text typecast of an extant interval value. — creating an interval value by adding/subtracting an extant interval value to/from another — creating an interval value by multiplying or dividing an extant interval value by a (real) number — creating an interval value by subtracting a pair of moments of the same data type (timestamptz, plain timestamp, or time) — creating a new moment value by adding or subtracting an extant interval value to an extant moment value of the same data type. — creating an interval value by applying justify_hours(i), justify_days(i), and justify_interval(i) to an extant interval value, i. — creating a double precision value by applying extract(epoch from i) to an extant interval value, i. — evaluating inequality and equality tests to compare two extant interval values. Notice that, for example, this test: select interval '1.3443 years' as i1, interval '1 years' * 1.3443 as i2; conflates three things: converting an interval literal to a [mm, dd, ss] tuple; multiplying a [mm, dd, ss] tuple by a real number; and converting a [mm, dd, ss] tuple to a text representation. Similarly, this test: select interval '1.3443 years' < interval '1 years' * 1.3443; conflates three things: converting an interval literal to a [mm, dd, ss] tuple; multiplying a [mm, dd, ss] tuple by a real number; and inequality comparison of two [mm, dd, ss] two [mm, dd, ss] tuples. As far as I’ve been able, the PG documentation doesn’t do a good job of defining the semantics of any of these operations. Some (like the “justify” functions” are sketched reasonably well. Others, like interval multiplication, are entirely undefined. This makes discussion of simple test like the two I showed immediately above hard. It also makes any discussion of correctness, possible bugs, and proposed implementation changes very difficult. Further, it also makes it hard to see how tests for application code that uses any of these operations can be designed. The normal approach relies on testing that you get what you expect. But here, you don't know what to expect—unless (as I’ve done) you first assert hypotheses for the undefined operations and test them with programmed simulations. Of course, this is, in general, an unreliable approach. The only way to know what code is intended to do is to read the prose specification that informs the implementation. I had forgotten one piece of the long history of this thread. Soon after I presented the testcase that folks agree shows a clear bug, I asked about the rules for creating the internal [mm, dd, ss] tuple from a text literal that uses real numbers for the fields. My tests showed two things: (1) an intuitively clear model for the spilldown of nonintegral months to days and then, in turn, of nonintegral days to seconds; and (2) a quirky r
Re: Removing "long int"-related limit on hash table sizes
Ranier Vilela writes: > I think int64 is in most cases the counterpart of *long* on Windows. I'm not particularly on board with s/long/int64/g as a universal solution. I think that most of these usages are concerned with memory sizes and would be better off as "size_t". We might need int64 in places where we're concerned with sums of memory usage across processes, or where the value needs to be allowed to be negative. So it'll take case-by-case analysis to do it right. BTW, one aspect of this that I'm unsure how to tackle is the common usage of "L" constants; in particular, "work_mem * 1024L" is a really common idiom that we'll need to get rid of. Not sure that grep will be a useful aid for finding those. regards, tom lane
Re: when the startup process doesn't (logging startup delays)
On Fri, Jul 23, 2021 at 04:09:47PM +0530, Nitin Jadhav wrote:
> > I think walkdir() should only call LogStartupProgress(FSYNC_IN_PROGRESS,
> > path);
> > when action == datadir_fsync_fname.
>
> I agree and fixed it.
I saw that you fixed it by calling InitStartupProgress() after the walkdir()
calls which do pre_sync_fname. So then walkdir is calling
LogStartupProgress(STARTUP_PROCESS_OP_FSYNC) even when it's not doing fsync,
and then LogStartupProgress() is returning because !AmStartupProcess().
That seems indirect, fragile, and confusing. I suggest that walkdir() should
take an argument for which operation to pass to LogStartupProgress(). You can
pass a special enum for cases where nothing should be logged, like
STARTUP_PROCESS_OP_NONE.
On Wed, Jul 21, 2021 at 04:47:32PM +0530, Bharath Rupireddy wrote:
> 1) I still don't see the need for two functions LogStartupProgress and
> LogStartupProgressComplete. Most of the code is duplicate. I think we
> can just do it with a single function something like [1]:
I agree that one function can do this more succinctly. I think it's best to
use a separate enum value for START operations and END operations.
switch(operation)
{
case STARTUP_PROCESS_OP_SYNCFS_START:
ereport(...);
break;
case STARTUP_PROCESS_OP_SYNCFS_END:
ereport(...);
break;
case STARTUP_PROCESS_OP_FSYNC_START:
ereport(...);
break;
case STARTUP_PROCESS_OP_FSYNC_END:
ereport(...);
break;
...
--
Justin
Re: Removing "long int"-related limit on hash table sizes
Em dom., 25 de jul. de 2021 às 13:28, Tom Lane escreveu: > Andres Freund writes: > > On 2021-07-23 17:15:24 -0400, Tom Lane wrote: > >> That's because they spill to disk where they did not before. The easy > >> answer of "raise hash_mem_multiplier" doesn't help, because on Windows > >> the product of work_mem and hash_mem_multiplier is clamped to 2GB, > >> thanks to the ancient decision to do a lot of memory-space-related > >> calculations in "long int", which is only 32 bits on Win64. > > > We really ought to just remove every single use of long. > > I have no objection to that as a long-term goal. But I'm not volunteering > to do all the work, and in any case it wouldn't be a back-patchable fix. > I'm a volunteer, if you want to work together. I think int64 is in most cases the counterpart of *long* on Windows. regards, Ranier Vilela
Re: Planning counters in pg_stat_statements (using pgss_store)
Le lun. 26 juil. 2021 à 00:59, Tom Lane a écrit : > Julien Rouhaud writes: > > On Sun, Jul 25, 2021 at 12:03:25PM -0400, Tom Lane wrote: > > > Would it be worth to split the query for the prepared statement row vs > the rest > > to keep the full "plans" coverage when possible? > > +1, the same thought occurred to me later. Also, if we're making > it specific to the one PREPARE example, we could get away with > checking "plans >= 2 AND plans <= calls", with a comment like > "we expect at least one replan event, but there could be more". > Do you want to prepare a patch? > Sure, I will work on that tomorrow! >
Re: Planning counters in pg_stat_statements (using pgss_store)
Julien Rouhaud writes: > On Sun, Jul 25, 2021 at 12:03:25PM -0400, Tom Lane wrote: >> So AFAICS this test is inherently unstable and there is no code bug >> to be fixed. We could drop the "plans" column from this query, or >> print something approximate like "plans > 0 AND plans <= calls". >> Thoughts? > I think we should go with the latter. Checking for a legit value, even if > it's > a bit imprecise is still better than nothing. > Would it be worth to split the query for the prepared statement row vs the > rest > to keep the full "plans" coverage when possible? +1, the same thought occurred to me later. Also, if we're making it specific to the one PREPARE example, we could get away with checking "plans >= 2 AND plans <= calls", with a comment like "we expect at least one replan event, but there could be more". Do you want to prepare a patch? regards, tom lane
Re: Planning counters in pg_stat_statements (using pgss_store)
On Sun, Jul 25, 2021 at 12:03:25PM -0400, Tom Lane wrote: > The cause of the failure of course is that cache clobbering includes > plan cache clobbering, so that the prepared statement's plan is > remade each time it's used, not only twice as the test expects. > However, remembering that cache flushes can happen for other reasons, > it's my guess that this test case would prove unstable in the buildfarm > even without considering the CLOBBER_CACHE_ALWAYS members. For example, > a background autovacuum hitting the "test" table at just the right time > would result in extra planning. We haven't seen that because the > buildfarm's not running this test, but that's about to change. Indeed. > So AFAICS this test is inherently unstable and there is no code bug > to be fixed. We could drop the "plans" column from this query, or > print something approximate like "plans > 0 AND plans <= calls". > Thoughts? I think we should go with the latter. Checking for a legit value, even if it's a bit imprecise is still better than nothing. Would it be worth to split the query for the prepared statement row vs the rest to keep the full "plans" coverage when possible?
Re: Removing "long int"-related limit on hash table sizes
Andres Freund writes: > On 2021-07-23 17:15:24 -0400, Tom Lane wrote: >> That's because they spill to disk where they did not before. The easy >> answer of "raise hash_mem_multiplier" doesn't help, because on Windows >> the product of work_mem and hash_mem_multiplier is clamped to 2GB, >> thanks to the ancient decision to do a lot of memory-space-related >> calculations in "long int", which is only 32 bits on Win64. > We really ought to just remove every single use of long. I have no objection to that as a long-term goal. But I'm not volunteering to do all the work, and in any case it wouldn't be a back-patchable fix. I feel that we do need to do something about this performance regression in v13. > Hm. I wonder if we would avoid some overflow dangers on 32bit systems if > we made get_hash_memory_limit() and the relevant variables 64 bit, > rather than 32bit / size_t. E.g. No, I don't like that. Using size_t for memory-size variables is good discipline. Moreover, I'm not convinced that even with 64-bit ints, overflow would be impossible in all the places I fixed here. They're multiplying several potentially very large values (one of which is a float). I think this is just plain sloppy coding, independently of which bit-width you choose to be sloppy in. >> +skew_mcvs = skew_mcvs * SKEW_HASH_MEM_PERCENT / 100; > I always have to think about the evaluation order of things like this > (it's left to right for these), so I'd prefer parens around the > multiplication. I did wonder briefly whether the SKEW_HASH_MEM_PERCENT / > 100 just evaluates to 0... OK, will do. I see your point, because I'd sort of instinctively wanted to write that as skew_mcvs *= SKEW_HASH_MEM_PERCENT / 100; which of course would not work. Thanks for looking at the code. regards, tom lane
Re: [PoC] Improve dead tuple storage for lazy vacuum
Hi, I've dreamed to write more compact structure for vacuum for three years, but life didn't give me a time to. Let me join to friendly competition. I've bet on HATM approach: popcount-ing bitmaps for non-empty elements. Novelties: - 32 consecutive pages are stored together in a single sparse array (called "chunks"). Chunk contains: - its number, - 4 byte bitmap of non-empty pages, - array of non-empty page headers 2 byte each. Page header contains offset of page's bitmap in bitmaps container. (Except if there is just one dead tuple in a page. Then it is written into header itself). - container of concatenated bitmaps. Ie, page metadata overhead varies from 2.4byte (32pages in single chunk) to 18byte (1 page in single chunk) per page. - If page's bitmap is sparse ie contains a lot of "all-zero" bytes, it is compressed by removing zero byte and indexing with two-level bitmap index. Two-level index - zero bytes in first level are removed using second level. It is mostly done for 32kb pages, but let it stay since it is almost free. - If page's bitmaps contains a lot of "all-one" bytes, it is inverted and then encoded as sparse. - Chunks are allocated with custom "allocator" that has no per-allocation overhead. It is possible because there is no need to perform "free": allocator is freed as whole at once. - Array of pointers to chunks is also bitmap indexed. It saves cpu time when not every 32 consecutive pages has at least one dead tuple. But consumes time otherwise. Therefore additional optimization is added to quick skip lookup for first non-empty run of chunks. (Ahhh, I believe this explanation is awful). Andres Freund wrote 2021-07-20 02:49: Hi, On 2021-07-19 15:20:54 +0900, Masahiko Sawada wrote: BTW is the implementation of the radix tree approach available somewhere? If so I'd like to experiment with that too. > > I have toyed with implementing adaptively large radix nodes like > proposed in https://db.in.tum.de/~leis/papers/ART.pdf - but haven't > gotten it quite working. That seems promising approach. I've since implemented some, but not all of the ideas of that paper (adaptive node sizes, but not the tree compression pieces). E.g. for select prepare( 100, -- max block 20, -- # of dead tuples per page 10, -- dead tuples interval within a page 1 -- page inteval ); attach sizeshuffledordered array69 ms 120 MB 84.87 s 8.66 s intset 173 ms 65 MB 68.82 s 11.75 s rtbm201 ms 67 MB 11.54 s 1.35 s tbm 232 ms 100 MB 8.33 s 1.26 s vtbm162 ms 58 MB 10.01 s 1.22 s radix88 ms 42 MB 11.49 s 1.67 s and for select prepare( 100, -- max block 10, -- # of dead tuples per page 1, -- dead tuples interval within a page 1 -- page inteval ); attach sizeshuffledordered array24 ms 60MB 3.74s1.02 s intset 97 ms 49MB 3.14s0.75 s rtbm138 ms 36MB 0.41s0.14 s tbm 198 ms 101MB 0.41s0.14 s vtbm118 ms 27MB 0.39s0.12 s radix33 ms 10MB 0.28s0.10 s (this is an almost unfairly good case for radix) Running out of time to format the results of the other testcases before I have to run, unfortunately. radix uses 42MB both in test case 3 and 4. My results (Ubuntu 20.04 Intel Core i7-1165G7): Test1. select prepare(100, 10, 20, 1); -- original attach size shuffled array 29ms60MB 93.99s intset 93ms49MB 80.94s rtbm 171ms67MB 14.05s tbm238ms 100MB8.36s vtbm 148ms59MB9.12s radix 100ms42MB 11.81s svtm75ms29MB8.90s select prepare(100, 20, 10, 1); -- Andres's variant attach size shuffled array 61ms 120MB 111.91s intset 163ms66MB 85.00s rtbm 236ms67MB 10.72s tbm290ms 100MB8.40s vtbm 190ms59MB9.28s radix 117ms42MB 12.00s svtm98ms29MB8.77s Test2. select prepare(100, 10, 1, 1); attach size shuffled array 31ms60MB4.68s intset 97ms49MB4.03s rtbm 163ms36MB0.42s tbm240ms 100MB0.42s vtbm 136ms27MB0.36s radix 60ms10MB0.72s svtm39ms 6MB0.19s (Bad radix result probably due to smaller cache in notebook's CPU ?) Test3 select prepare(100, 2, 100, 1); attach size shuffled array6ms12MB 53.42s intset 23ms16MB 54.99s rtbm 115ms38MB8.19s tbm186ms 100MB8.37s vtbm 105ms59MB9.08s radix 64ms42MB 10.41s svtm73ms10MB7.49s Test4 select prepare(100, 100, 1, 1); attach size shuffled array 304ms 600MB 75.12s intset 775ms98MB 47.49s rtbm 356ms38MB4.11s tbm539ms 100MB4.20s vtbm 493ms42MB4.44s radix 263ms42MB6.05s svtm 360ms 8MB3.49s Therefore Specialized
Re: Planning counters in pg_stat_statements (using pgss_store)
[ blast from the past department ] Fujii Masao writes: > Finally I pushed the patch! > Many thanks for all involved in this patch! It turns out that the regression test outputs from this patch are unstable under debug_discard_caches (nee CLOBBER_CACHE_ALWAYS). You can easily check this in HEAD or v14, with something along the lines of $ cd ~/pgsql/contrib/pg_stat_statements $ echo "debug_discard_caches = 1" >/tmp/temp_config $ TEMP_CONFIG=/tmp/temp_config make check and what you will get is a diff like this: SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C"; query | plans | calls | rows ... - PREPARE prep1 AS SELECT COUNT(*) FROM test | 2 | 4 |4 + PREPARE prep1 AS SELECT COUNT(*) FROM test | 4 | 4 |4 The reason we didn't detect this long since is that the buildfarm client script fails to run "make check" for contrib modules that are marked NO_INSTALLCHECK, so that pg_stat_statements (among others) has received precisely zero buildfarm testing. Buildfarm member sifaka is running an unreleased version of the script that fixes that oversight, and when I experimented with turning on debug_discard_caches, I got this failure, as shown at [1]. The cause of the failure of course is that cache clobbering includes plan cache clobbering, so that the prepared statement's plan is remade each time it's used, not only twice as the test expects. However, remembering that cache flushes can happen for other reasons, it's my guess that this test case would prove unstable in the buildfarm even without considering the CLOBBER_CACHE_ALWAYS members. For example, a background autovacuum hitting the "test" table at just the right time would result in extra planning. We haven't seen that because the buildfarm's not running this test, but that's about to change. So AFAICS this test is inherently unstable and there is no code bug to be fixed. We could drop the "plans" column from this query, or print something approximate like "plans > 0 AND plans <= calls". Thoughts? regards, tom lane [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sifaka&dt=2021-07-24%2023%3A53%3A52
Re: Configure with thread sanitizer fails the thread test
Hi, Alvaro and all, > this patch hasn't been posted by the author so let's assume > they're not authorizing them to use it. Not sure what you mean. I am the author and I authorize anyone to do whatever they want with it. > Otherwise, why wouldn't they just post it here instead of doing the absurdly convoluted dance of a github PR? Well, for me submitting a github PR is a well-established and easy-to-do procedure which is the same for all libraries. Posting to a new mailing list definitely is not. I've spent around 15 minutes trying to do a proper reply to the thread and did not succeed. Another reason to post a PR is that we consume libpq via conan, and releasing a new revision of the conan recipe for the same library version is a relatively fast and well-defined process. While waiting for a new version of a library with a patch depends heavily on a particular library. I am not aware of the release cadence of libpq. Anyway, I am very glad there is swift feedback from you guys and I am looking forward to your comments on the proper way to fix the issue! - Best regards, Mikhail Matrosov
Re: Rename of triggers for partitioned tables
Alvaro Herrera writes: > I pushed this patch with some minor corrections (mostly improved the > message wording.) Coverity does not like this bit, and I fully agree: /srv/coverity/git/pgsql-git/postgresql/src/backend/commands/trigger.c: 1639 in renametrig_partition() >>> CID 1489387: Incorrect expression (ASSERT_SIDE_EFFECT) >>> Argument "found++" of Assert() has a side effect. The containing >>> function might work differently in a non-debug build. 1639Assert(found++ <= 0); Perhaps there's no actual bug there, but it's still horrible coding. For one thing, the implication that found could be negative is extremely confusing to readers. A boolean might be better. However, I wonder why you bothered with a flag in the first place. The usual convention if we know there can be only one match is to just not write a loop at all, with a suitable comment, like this pre-existing example elsewhere in trigger.c: /* There should be at most one matching tuple */ if (HeapTupleIsValid(tuple = systable_getnext(tgscan))) If you're not quite convinced there can be only one match, then it still shouldn't be an Assert --- a real test-and-elog would be better. regards, tom lane
Re: proposal: plpgsql: special comments that will be part of AST
Hi > > some like: > > //* plpgsql_check: OFF *// > RAISE NOTICE '%', r.x > > or second design > > b) can we introduce some flag for plpgsql_parser, that allows storing > comments in AST (it will not be default). > > so "-- plpgsql_check: OFF " will work for my purpose, and I don't need > any special syntax. > > Comments, notes? > When I started work on PoC I found that it was not a good idea. Comments can be everywhere, but it is not possible to enhance plpgsql's gram in this way. So this is not an way Regards Pavel > Pavel > > https://github.com/okbob/plpgsql_check >
Re: [PROPOSAL] new diagnostic items for the dynamic sql
ne 25. 7. 2021 v 12:52 odesílatel Dinesh Chemuduru
napsal:
> On Sat, 17 Jul 2021 at 01:29, Pavel Stehule
> wrote:
>
>> Hi
>>
>> pá 16. 7. 2021 v 21:47 odesílatel Dinesh Chemuduru <
>> dinesh.ku...@migops.com> napsal:
>>
>>> Hi Everyone,
>>>
>>> We would like to propose the below 2 new plpgsql diagnostic items,
>>> related to parsing. Because, the current diag items are not providing
>>> the useful diagnostics about the dynamic SQL statements.
>>>
>>> 1. PG_PARSE_SQL_STATEMENT (returns parse failed sql statement)
>>> 2. PG_PARSE_SQL_STATEMENT_POSITION (returns parse failed sql text cursor
>>> position)
>>>
>>> Consider the below example, which is an invalid SQL statement.
>>>
>>> postgres=# SELECT 1 JOIN SELECT 2;
>>> ERROR: syntax error at or near "JOIN"
>>> LINE 1: SELECT 1 JOIN SELECT 2;
>>> ^
>>> Here, there is a syntax error at JOIN clause,
>>> and also we are getting the syntax error position(^ symbol, the position
>>> of JOIN clause).
>>> This will be helpful, while dealing with long queries.
>>>
>>> Now, if we run the same statement as a dynamic SQL(by using EXECUTE >> statement>),
>>> then it seems we are not getting the text cursor position,
>>> and the SQL statement which is failing at parse level.
>>>
>>> Please find the below example.
>>>
>>> postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
>>> NOTICE: RETURNED_SQLSTATE 42601
>>> NOTICE: COLUMN_NAME
>>> NOTICE: CONSTRAINT_NAME
>>> NOTICE: PG_DATATYPE_NAME
>>> NOTICE: MESSAGE_TEXT syntax error at or near "JOIN"
>>> NOTICE: TABLE_NAME
>>> NOTICE: SCHEMA_NAME
>>> NOTICE: PG_EXCEPTION_DETAIL
>>> NOTICE: PG_EXCEPTION_HINT
>>> NOTICE: PG_EXCEPTION_CONTEXT PL/pgSQL function exec_me(text) line 18 at
>>> EXECUTE
>>> NOTICE: PG_CONTEXT PL/pgSQL function exec_me(text) line 21 at GET
>>> STACKED DIAGNOSTICS
>>> exec_me
>>> -
>>>
>>> (1 row)
>>>
>>> From the above results, by using all the existing diag items, we are
>>> unable to get the position of "JOIN" in the submitted SQL statement.
>>> By using these proposed diag items, we will be getting the required
>>> information,
>>> which will be helpful while running long SQL statements as dynamic SQL
>>> statements.
>>>
>>> Please find the below example.
>>>
>>> postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
>>> NOTICE: PG_PARSE_SQL_STATEMENT SELECT 1 JOIN SELECT 2
>>> NOTICE: PG_PARSE_SQL_STATEMENT_POSITION 10
>>> exec_me
>>> -
>>>
>>> (1 row)
>>>
>>> From the above results, by using these diag items,
>>> we are able to get what is failing and it's position as well.
>>> This information will be much helpful to debug the issue,
>>> while a long running SQL statement is running as a dynamic SQL statement.
>>>
>>> We are attaching the patch for this proposal, and will be looking for
>>> your inputs.
>>>
>>
>> +1 It is good idea. I am not sure if the used names are good. I propose
>>
>> PG_SQL_TEXT and PG_ERROR_LOCATION
>>
>> Regards
>>
>> Pavel
>>
>>
> Thanks Pavel,
>
> Sorry for the late reply.
>
> The proposed diag items are `PG_SQL_TEXT`, `PG_ERROR_LOCATION` are much
> better and generic.
>
> But, as we are only dealing with the parsing failure, I thought of adding
> that to the diag name.
>
I understand. But parsing is only one case - and these variables can be
used for any case. Sure, ***we don't want*** to have PG_PARSE_SQL_TEXT,
PG_ANALYZE_SQL_TEXT, PG_EXECUTION_SQL_TEXT ...
The idea is good, and you found the case, where it has benefits for users.
Naming is hard.
Regards
Pavel
> Regards,
> Dinesh Kumar
>
>
>>
>>
>>> Regards,
>>> Dinesh Kumar
>>>
>>
Re: [PROPOSAL] new diagnostic items for the dynamic sql
On Sat, 17 Jul 2021 at 01:29, Pavel Stehule wrote:
> Hi
>
> pá 16. 7. 2021 v 21:47 odesílatel Dinesh Chemuduru <
> dinesh.ku...@migops.com> napsal:
>
>> Hi Everyone,
>>
>> We would like to propose the below 2 new plpgsql diagnostic items,
>> related to parsing. Because, the current diag items are not providing
>> the useful diagnostics about the dynamic SQL statements.
>>
>> 1. PG_PARSE_SQL_STATEMENT (returns parse failed sql statement)
>> 2. PG_PARSE_SQL_STATEMENT_POSITION (returns parse failed sql text cursor
>> position)
>>
>> Consider the below example, which is an invalid SQL statement.
>>
>> postgres=# SELECT 1 JOIN SELECT 2;
>> ERROR: syntax error at or near "JOIN"
>> LINE 1: SELECT 1 JOIN SELECT 2;
>> ^
>> Here, there is a syntax error at JOIN clause,
>> and also we are getting the syntax error position(^ symbol, the position
>> of JOIN clause).
>> This will be helpful, while dealing with long queries.
>>
>> Now, if we run the same statement as a dynamic SQL(by using EXECUTE > statement>),
>> then it seems we are not getting the text cursor position,
>> and the SQL statement which is failing at parse level.
>>
>> Please find the below example.
>>
>> postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
>> NOTICE: RETURNED_SQLSTATE 42601
>> NOTICE: COLUMN_NAME
>> NOTICE: CONSTRAINT_NAME
>> NOTICE: PG_DATATYPE_NAME
>> NOTICE: MESSAGE_TEXT syntax error at or near "JOIN"
>> NOTICE: TABLE_NAME
>> NOTICE: SCHEMA_NAME
>> NOTICE: PG_EXCEPTION_DETAIL
>> NOTICE: PG_EXCEPTION_HINT
>> NOTICE: PG_EXCEPTION_CONTEXT PL/pgSQL function exec_me(text) line 18 at
>> EXECUTE
>> NOTICE: PG_CONTEXT PL/pgSQL function exec_me(text) line 21 at GET
>> STACKED DIAGNOSTICS
>> exec_me
>> -
>>
>> (1 row)
>>
>> From the above results, by using all the existing diag items, we are
>> unable to get the position of "JOIN" in the submitted SQL statement.
>> By using these proposed diag items, we will be getting the required
>> information,
>> which will be helpful while running long SQL statements as dynamic SQL
>> statements.
>>
>> Please find the below example.
>>
>> postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
>> NOTICE: PG_PARSE_SQL_STATEMENT SELECT 1 JOIN SELECT 2
>> NOTICE: PG_PARSE_SQL_STATEMENT_POSITION 10
>> exec_me
>> -
>>
>> (1 row)
>>
>> From the above results, by using these diag items,
>> we are able to get what is failing and it's position as well.
>> This information will be much helpful to debug the issue,
>> while a long running SQL statement is running as a dynamic SQL statement.
>>
>> We are attaching the patch for this proposal, and will be looking for
>> your inputs.
>>
>
> +1 It is good idea. I am not sure if the used names are good. I propose
>
> PG_SQL_TEXT and PG_ERROR_LOCATION
>
> Regards
>
> Pavel
>
>
Thanks Pavel,
Sorry for the late reply.
The proposed diag items are `PG_SQL_TEXT`, `PG_ERROR_LOCATION` are much
better and generic.
But, as we are only dealing with the parsing failure, I thought of adding
that to the diag name.
Regards,
Dinesh Kumar
>
>
>> Regards,
>> Dinesh Kumar
>>
>
Re: Avoiding data loss with synchronous replication
> 25 июля 2021 г., в 05:29, Andres Freund написал(а): > > Hi, > > On 2021-07-24 15:53:15 +0500, Andrey Borodin wrote: >> Are there any other problems with blocking cancels? > > Unless you have commandline access to the server, it's not hard to get > into a situation where you can't change the configuration setting > because all connections are hanging, and you can't even log in to do an > ALTER SERVER etc. You can't kill applications to kill the connection, > because they will just continue to hang. Hmm, yes, it's not hard to get to this situation. Intentionally. But what would be setup to get into such troubles? Setting sync rep, but not configuring HA tool? In normal circumstances HA cluster is not configured to allow this. Normally hanging commits are part of the failover. Somewhere new primary server is operating. You have to find commandline access to the server to execute pg_rewind, and join this node to cluster again as a standby. Anyway it's a good idea to set up superuser_reserved_connections for administrative intervention [0]. I like the idea of transferring transaction locks somewhere until synchronous_commit requirements are satisfied. It makes us closer to making this locks durable to survive restart. But, IMO, the complexity and potentially dangerous conditions outweigh the benefits of this approach easily. Thanks! Best regards, Andrey Borodin. [0]
Re: Maintain the pathkesy for subquery from outer side information
> I think that in cases where there's not a semantic hazard involved, > we'd usually have pulled up the subquery so that this is all moot > anyway. > I get your point with this statement. Things driven by this idea look practical and lucky. But for the UniqueKey stuff, we are not that lucky. SELECT pk FROM t; -- Maintain the UniqueKey would be not necessary. However SELECT DISTINCT pk FROM (SELECT volatile_f(a), pk from t) WHERE ..; Maintaining the UniqueKey in subquery is necessary since it is useful outside. -- Best Regards Andy Fan (https://www.aliyun.com/)
