Re: Crash after a call to pg_backup_start()
On 2022-Oct-21, Michael Paquier wrote:
> On Fri, Oct 21, 2022 at 05:53:25PM +0800, Richard Guo wrote:
> > /* These conditions can not be both true */
>
> If you do that, it would be a bit easier to read as of the following
> assertion instead? Like:
> Assert(!during_backup_start ||
>sessionBackupState == SESSION_BACKUP_NONE);
My intention here was that the Assert should be inside the block, that
is, we already know that at least one is true, and we want to make sure
that they are not *both* true.
AFAICT the attached patch also fixes the bug without making the assert
weaker.
--
Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
>From 710eef9259f4286f8d8660ac1dd898323205a037 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Sat, 22 Oct 2022 09:54:24 +0200
Subject: [PATCH] Fix misplaced assertion
---
src/backend/access/transam/xlog.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index dea978a962..fb3fbcd2be 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8841,12 +8841,12 @@ do_pg_abort_backup(int code, Datum arg)
{
bool during_backup_start = DatumGetBool(arg);
- /* Only one of these conditions can be true */
- Assert(during_backup_start ^
- (sessionBackupState == SESSION_BACKUP_RUNNING));
-
if (during_backup_start || sessionBackupState != SESSION_BACKUP_NONE)
{
+ /* We should be here only by one of these reasons, never both */
+ Assert(during_backup_start ^
+ (sessionBackupState == SESSION_BACKUP_RUNNING));
+
WALInsertLockAcquireExclusive();
Assert(XLogCtl->Insert.runningBackups > 0);
XLogCtl->Insert.runningBackups--;
--
2.30.2
Re: Crash after a call to pg_backup_start()
On Sat, Oct 22, 2022 at 1:26 PM Alvaro Herrera wrote: > > On 2022-Oct-21, Michael Paquier wrote: > > > On Fri, Oct 21, 2022 at 05:53:25PM +0800, Richard Guo wrote: > > > > /* These conditions can not be both true */ > > > > If you do that, it would be a bit easier to read as of the following > > assertion instead? Like: > > Assert(!during_backup_start || > >sessionBackupState == SESSION_BACKUP_NONE); > > My intention here was that the Assert should be inside the block, that > is, we already know that at least one is true, and we want to make sure > that they are not *both* true. > > AFAICT the attached patch also fixes the bug without making the assert > weaker. +/* We should be here only by one of these reasons, never both */ +Assert(during_backup_start ^ + (sessionBackupState == SESSION_BACKUP_RUNNING)); + What's the problem even if we're here when both of them are true? The runningBackups is incremented anyways, right? Why can't we just get rid of the Assert and treat during_backup_start as backup_marked_active_in_shmem or something like that to keep things simple? -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Missing update of all_hasnulls in BRIN opclasses
On 2022-Oct-22, Tomas Vondra wrote: > I wonder how to do this in a back-patchable way - we can't add > parameters to the opclass procedure, and the other solution seems to be > storing it right in the BrinMemTuple, somehow. But that's likely an ABI > break :-( Hmm, I don't see the ABI incompatibility. BrinMemTuple is an in-memory structure, so you can add new members at the end of the struct and it will pose no problems to existing code. > The only solution I can think of is actually passing it using all_nulls > and has_nulls - we could set both flags to true (which we never do now) > and teach the opclass that it signifies "empty" (and thus not to update > has_nulls after resetting all_nulls). > > Something like the attached (I haven't added any more tests, not sure > what would those look like - I can't think of a query testing this, > although maybe we could check how the flags change using pageinspect). I'll try to have a look at these patches tomorrow or on Monday. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "I suspect most samba developers are already technically insane... Of course, since many of them are Australians, you can't tell." (L. Torvalds)
Re: Crash after a call to pg_backup_start()
On 2022-Oct-22, Bharath Rupireddy wrote: > +/* We should be here only by one of these reasons, never both */ > +Assert(during_backup_start ^ > + (sessionBackupState == SESSION_BACKUP_RUNNING)); > + > > What's the problem even if we're here when both of them are true? In what case should we be there with both conditions true? > The runningBackups is incremented anyways, right? In the current code, yes, but it seems to be easier to reason about if we know precisely why we're there and whether we should be running the cleanup or not. Otherwise we might end up with a bug where we run the function but it doesn't do anything because we failed to understand the preconditions. At the very least, this forces a developer changing this code to think through it. > Why can't we just get rid of the Assert and treat during_backup_start > as backup_marked_active_in_shmem or something like that to keep things > simple? Why is that simpler? -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "La verdad no siempre es bonita, pero el hambre de ella sí"
Re: Crash after a call to pg_backup_start()
On Sat, Oct 22, 2022 at 1:56 PM Alvaro Herrera wrote: > > > Why can't we just get rid of the Assert and treat during_backup_start > > as backup_marked_active_in_shmem or something like that to keep things > > simple? > > Why is that simpler? IMO, the assertion looks complex there and thinking if we can remove it. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Pluggable toaster
Hi Nikita, > Aleksander, we have had this in mind while developing this feature, and have > checked it. Just a slight modification is needed > to make it work with Pluggable Storage (Access Methods) API. Could you please clarify this a little from the architectural point of view? Let's say company A implements some specific TableAM (in-memory / the one that uses undo logging / etc). Company B implements an alternative TOAST mechanism. How the TOAST extension is going to work without knowing any specifics of the TableAM the user chooses for the given relation, and vice versa? How one of the extensions is going to upgrade / downgrade between the versions without knowing any implementation details of another? -- Best regards, Aleksander Alekseev
Re: Avoid memory leaks during base backups
On Fri, Oct 21, 2022 at 09:02:04PM +0530, Bharath Rupireddy wrote: > After all, that is what is being discussed here; what if palloc down > below fails and they're not reset to NULL after MemoryContextReset()? It does not seem to matter much to me for that, so left these as proposed. > On Fri, Oct 21, 2022 at 12:11 PM Kyotaro Horiguchi > wrote: >> I think the "less" is somewhat obscure. I feel we should be more >> explicitly. And we don't need to put emphasis on "leak". I recklessly >> propose this as the draft. > > I tried to put it simple, please see the attached v10. I'll leave it > to the committer's discretion for better wording. I am still not sure what "less" means when referring to a "memory context". Anyway, I have gone through the comments and finished with something much more simplified, and applied the whole. -- Michael signature.asc Description: PGP signature
pg_dump test: Make concatenated create_sql commands more readable
When the pg_dump 002_pg_dump.pl test generates the command to load the
schema, it does
# Add terminating semicolon
$create_sql{$test_db} .= $tests{$test}->{create_sql} . ";";
In some cases, this creates a duplicate semicolon, but more importantly,
this doesn't add any newline. So if you look at the result in either
the server log or in tmp_check/log/regress_log_002_pg_dump, it looks
like a complete mess. The attached patch makes the output look cleaner
for manual inspection: add semicolon only if necessary, and add two
newlines.From 28e70332050e81c3faebc1bd0fe31e5863adcb78 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Sat, 22 Oct 2022 12:29:26 +0200
Subject: [PATCH] pg_dump test: Make concatenated create_sql commands more
readable
---
src/bin/pg_dump/t/002_pg_dump.pl | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index a869321cdfc3..5df5a0ad59ef 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -3984,8 +3984,12 @@
next;
}
- # Add terminating semicolon
- $create_sql{$test_db} .= $tests{$test}->{create_sql} . ";";
+ # Normalize command ending: strip all line endings, add
+ # semicolon if missing, add two newlines.
+ my $create_sql = $tests{$test}->{create_sql};
+ chomp $create_sql;
+ $create_sql .= ';' unless substr($create_sql, -1) eq ';';
+ $create_sql{$test_db} .= $create_sql . "\n\n";
}
}
--
2.37.3
Re: Logical WAL sender unresponsive during decoding commit
On Thu, Oct 20, 2022 at 7:17 PM Robert Haas wrote: > > On Thu, Oct 20, 2022 at 1:37 AM Amit Kapila wrote: > > On Thu, Oct 20, 2022 at 5:17 AM Robert Haas wrote: > > > > Pushed. > > > > > > I think this was a good change, but there's at least one other problem > > > here: within ReorderBufferRestoreChanges, the while (restored < > > > max_changes_in_memory && *segno <= last_segno) doesn't seem to contain > > > a CFI. Note that this can loop either by repeatedly failing to open a > > > file, or by repeatedly reading from a file and passing the data read > > > to ReorderBufferRestoreChange. So I think there should just be a CFI > > > at the top of this loop to make sure both cases are covered. > > > > Agreed. The failures due to file operations can make this loop > > unpredictable in terms of time, so it is a good idea to have CFI at > > the top of this loop. > > > > I can take care of this unless there are any objections or you want to > > do it. We have backpatched the previous similar change, so I think we > > should backpatch this as well. What do you think? > > Please go ahead. +1 for back-patching. > Yesterday, I have pushed this change. -- With Regards, Amit Kapila.
Re: Missing update of all_hasnulls in BRIN opclasses
On 10/22/22 10:00, Alvaro Herrera wrote:
> On 2022-Oct-22, Tomas Vondra wrote:
>
>> I wonder how to do this in a back-patchable way - we can't add
>> parameters to the opclass procedure, and the other solution seems to be
>> storing it right in the BrinMemTuple, somehow. But that's likely an ABI
>> break :-(
>
> Hmm, I don't see the ABI incompatibility. BrinMemTuple is an in-memory
> structure, so you can add new members at the end of the struct and it
> will pose no problems to existing code.
>
But we're not passing BrinMemTuple to the opclass procedures - we're
passing a pointer to BrinValues, so we'd have to add the flag there. And
we're storing an array of those, so adding a field may shift the array
even if you add it at the end. Not sure if that's OK or not.
>> The only solution I can think of is actually passing it using all_nulls
>> and has_nulls - we could set both flags to true (which we never do now)
>> and teach the opclass that it signifies "empty" (and thus not to update
>> has_nulls after resetting all_nulls).
>>
>> Something like the attached (I haven't added any more tests, not sure
>> what would those look like - I can't think of a query testing this,
>> although maybe we could check how the flags change using pageinspect).
>
> I'll try to have a look at these patches tomorrow or on Monday.
>
I was experimenting with this a bit more, and unfortunately the latest
patch is still a few bricks shy - it did fix this particular issue, but
there were other cases that remained/got broken. See the first patch,
that adds a bunch of pageinspect tests testing different combinations.
After thinking about it a bit more, I think we can't quite fix this at
the opclass level, so the yesterday's patches are wrong. Instead, this
should be fixed in values_add_to_range() - the whole trick is we need to
remember the range was empty at the beginning, and only set the flag
when allnulls is false.
The reworked patch does that. And we can use the same logic (both flags
set mean no tuples were added to the range) when building the index, a
separate flag is not needed.
This slightly affects existing regression tests, because we won't create
any ranges for empty table (now we created one, because we initialized a
tuple in brinbuild and then wrote it to disk). This means that
brin_summarize_range now returns 0, but I think that's fine.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL CompanyFrom 5456cf819426d3f90c004f673dfc863903e568a1 Mon Sep 17 00:00:00 2001
From: Tomas Vondra
Date: Sat, 22 Oct 2022 12:47:33 +0200
Subject: [PATCH 1/2] pageinspect brinbugs test
---
contrib/pageinspect/Makefile | 2 +-
contrib/pageinspect/expected/brinbugs.out | 222 ++
contrib/pageinspect/sql/brinbugs.sql | 114 +++
3 files changed, 337 insertions(+), 1 deletion(-)
create mode 100644 contrib/pageinspect/expected/brinbugs.out
create mode 100644 contrib/pageinspect/sql/brinbugs.sql
diff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile
index 5c0736564ab..92305e981f7 100644
--- a/contrib/pageinspect/Makefile
+++ b/contrib/pageinspect/Makefile
@@ -21,7 +21,7 @@ DATA = pageinspect--1.9--1.10.sql pageinspect--1.8--1.9.sql \
pageinspect--1.0--1.1.sql
PGFILEDESC = "pageinspect - functions to inspect contents of database pages"
-REGRESS = page btree brin gin gist hash checksum oldextversions
+REGRESS = page btree brin gin gist hash checksum oldextversions brinbugs
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/contrib/pageinspect/expected/brinbugs.out b/contrib/pageinspect/expected/brinbugs.out
new file mode 100644
index 000..23843caa138
--- /dev/null
+++ b/contrib/pageinspect/expected/brinbugs.out
@@ -0,0 +1,222 @@
+create extension pageinspect;
+create table t (a int, b int);
+create index on t using brin (a, b);
+-- both columns should have has_nulls=false and [1,1] range
+truncate t;
+insert into t values (1,1);
+vacuum full t;
+select * from brin_page_items(get_raw_page('t_a_b_idx', 2), 't_a_b_idx'::regclass);
+ itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | value
++++--+--+-+--
+ 1 | 0 | 1 | f| f| f | {1 .. 1}
+ 1 | 0 | 2 | f| f| f | {1 .. 1}
+(2 rows)
+
+-- first column should have all_nulls=true, second has_nulls=false and [1,1] range
+truncate t;
+insert into t values (null, 1);
+vacuum full t;
+select * from brin_page_items(get_raw_page('t_a_b_idx', 2), 't_a_b_idx'::regclass);
+ itemoffset | blknum | attnum | allnulls | hasnulls | placeholder | value
++++--+--+-+--
+ 1 | 0 | 1 | t| f| f |
+ 1 | 0 | 2 | f| f| f | {1 .. 1}
+(2 rows)
+
+-- both columns sho
Re: Allow WindowFuncs prosupport function to use more optimal WindowClause options
On 10/20/22 22:02, David Rowley wrote: On Thu, 13 Oct 2022 at 13:34, David Rowley wrote: So it looks like the same can be done for rank() and dense_rank() too. I've added support for those in the attached. The attached adds support for percent_rank(), cume_dist() and ntile(). Shouldn't it be able to detect that these two windows are the same and only do one WindowAgg pass? explain (verbose, costs off) select row_number() over w1, lag(amname) over w2 from pg_am window w1 as (order by amname), w2 as (w1 rows unbounded preceding) ; QUERY PLAN - WindowAgg Output: (row_number() OVER (?)), lag(amname) OVER (?), amname -> WindowAgg Output: amname, row_number() OVER (?) -> Sort Output: amname Sort Key: pg_am.amname -> Seq Scan on pg_catalog.pg_am Output: amname (9 rows) -- Vik Fearing
Interesting areas for beginners
Hi hackers. Can you please share some areas that would be good to start contributing? Some months ago I've got my first patch accept [1], and I'm looking to try to make other contributions. Thanks in advance! [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=6a1f082abac9da756d473e16238a906ca5a592dc -- Matheus Alcantara
Re: Pluggable toaster
Hi! Aleksander, this is a good question. If I understood you correctly, you mean that the alternative TOAST mechanism B is using a specific Table AM A? Pluggable TOAST API was designed with storage flexibility in mind, and Custom TOAST mechanics is free to use any storage methods - we've tested it with some custom Toaster, because it is completely hidden from the caller, and is not limited to Heap, though extensions' interdependencies is a very tricky question, and surely not the one to be answered quickly. Still, I have good news on this topic - I'm currently re-working Pluggable TOAST in a more OOP-correct way, generalizing Table to Toaster relation from column attribute and reloptions with separate catalog table describing Relation,Toaster and TOAST storage entities relations, with lazy TOAST Tables creation for the Generic Toaster, and dropping the limits of 1 TOAST table per relation. In current implementation Toaster OID and TOAST relation ID are stored as a part of Relation, which is not the best solution, and leaves some Toaster's nuts and bolts open to AM that uses it, and we decided to hide this part into Toaster too. The next logical step is using Table AM API, if Table AM Routine is provided to Toaster, instead of direct calls to Heap AM methods. This was thought of in the following way: Table AM Routine is passed to Toaster as a parameter, and direct Heap calls are replaced with the TAM Routine calls. This is possible, but needs further investigation, because TOAST manipulations with data require, as it is seen from the first dive into TAM API, some extension of this API. I'll present the results of our research as soon as they're ready. On Sat, Oct 22, 2022 at 11:58 AM Aleksander Alekseev < aleksan...@timescale.com> wrote: > Hi Nikita, > > > Aleksander, we have had this in mind while developing this feature, and > have checked it. Just a slight modification is needed > > to make it work with Pluggable Storage (Access Methods) API. > > Could you please clarify this a little from the architectural point of > view? > > Let's say company A implements some specific TableAM (in-memory / the > one that uses undo logging / etc). Company B implements an alternative > TOAST mechanism. > > How the TOAST extension is going to work without knowing any specifics > of the TableAM the user chooses for the given relation, and vice > versa? How one of the extensions is going to upgrade / downgrade > between the versions without knowing any implementation details of > another? > > -- > Best regards, > Aleksander Alekseev > -- Regards, Nikita Malakhov Postgres Professional https://postgrespro.ru/
Re: Missing update of all_hasnulls in BRIN opclasses
Hi, Tomas: For 0002-fixup-brin-has_nulls-20221022.patch : + first_row = (bval->bv_hasnulls && bval->bv_allnulls); + + if (bval->bv_hasnulls && bval->bv_allnulls) It seems the if condition can be changed to `if (first_row)` which is more readable. Chhers
