Обсуждение: [MASSMAIL]post-freeze damage control

On Mon, Apr 8, 2024 at 10:42 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Can you elaborate, which patches you think were not ready? Let's make
> sure to capture any concrete concerns in the Open Items list.

Hi,

I'm moving this topic to a new thread for better visibility and less
admixture of concerns. I'd like to invite everyone here present to
opine on which patches we ought to be worried about. Here are a few
picks from me to start things off. My intention here is to convey "I
find these scary" rather than "these commits were irresponsible," so I
respectfully ask that you don't take the choice to list your patch
here as an attack, or the choice not to list your patch here as an
endorsement. I'm very happy if I'm wrong and these patches are not
actually scary. And likewise let's view any allegations of scariness
by others as a genuine attempt to figure out what might be broken,
rather than as an attempt to get anyone into trouble or whatever.

- As I wrote about separately, I'm concerned that the failover slots
stuff may not be in as good a shape as it needs to be for people to
get good use out of it.
http://postgr.es/m/CA+TgmoaA4oufUBR5B-4o83rnwGZ3zAA5UvwxDX=NjCm1TVgRsQ@mail.gmail.com

- I also wrote separately about the flurry of recent table AM changes.
http://postgr.es/m/CA+TgmoZpWB50GnJZYF1x8PenTqXDTFBH_Euu6ybGfzEy34o+5Q@mail.gmail.com

- The streaming read API stuff was all committed very last minute. I
think this should have been committed much sooner. It's probably not
going to break the world; it's more likely to have performance
consequences. But if it had gone in sooner, we'd have had more time to
figure that out.

- The heap pruning refactoring work, on the other hand, seems to be at
very significant risk of breaking the world. I don't doubt the
authors, but this is some very critical stuff and a lot of it happened
very quickly right at the end.

- Incremental backup could have all sorts of terrible data-corrupting
bugs. 55a5ee30cd65886ff0a2e7ffef4ec2816fbec273 was a full brown-paper
bag level fix, so the chances of there being further problems are
probably high.

- I'm slightly worried about the TID store work (ee1b30f12, 30e144287,
667e65aac35), perhaps for no reason. Actually, the results seem really
impressive, and a very quick look at some of the commits seemed kind
of reassuring. But, like the changes to pruning and freezing, this is
making some really fundamental changes to critical code. In this case,
it's code that has evolved very little over the years and seems to
have now evolved quite a lot.

- I couldn't understand why the "Operate
XLogCtl->log{Write,Flush}Result with atomics" code was correct when I
read it. That's not to say I know it to be incorrect. But, a spinlock
protecting two variables together guarantees more than atomic access
to each of those variables separately.

There's probably a lot more to worry about; there have been so
incredibly many changes recently. But this is as far as my speculation
extends, as of now. Comments welcome. Additional concerns also
welcome, as noted above. And again, no offense is intended to anyone.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, Apr 9, 2024 at 7:47 AM Robert Haas <robertmhaas@gmail.com> wrote:
> - The streaming read API stuff was all committed very last minute. I
> think this should have been committed much sooner. It's probably not
> going to break the world; it's more likely to have performance
> consequences. But if it had gone in sooner, we'd have had more time to
> figure that out.

OK, let me give an update on this work stream (pun intended).

One reason for the delay in committing was precisely that we were
fretting about regressions risks.  We tried pretty hard to identify
and grind down every regression we could find, and cases with
outstanding not-fully-understood or examined problems in that area
have been booted into the next cycle for more work: streaming bitmap
heapscan, several streaming vacuum patches, and more, basically things
that seem to have more complex interactions with other machinery.  The
only three places using streaming I/O that went in were:

041b9680: Use streaming I/O in ANALYZE.
b7b0f3f2: Use streaming I/O in sequential scans.
3a352df0: Use streaming I/O in pg_prewarm.

The first is a good first exercise in streaming random blocks;
hopefully no one would be too upset about an unexpected small
regression in ANALYZE, but as it happens it goes faster hot and cold
according to all reports.  The second is a good first exercise in
streaming sequential blocks, and it ranges from faster to no
regression, according to testing and reports.  The third is less
important, but it also goes faster.

Of those, streaming seq scan is clearly the most relevant to real
workloads that someone might be upset about, and I made a couple of
choices that you might say had damage control in mind:

* A conservative choice not to get into the business of the issuing
new hints to the kernel for random jumps in cold scans, even though we
think we probably should for better performance: more research needed
precisely to avoid unexpected interactions (cf the booted bitmap
heapscan where that sort of thing seems to be afoot).
* A GUC to turn off I/O combining if it somehow upsets your storage in
ways we didn't foresee (io_combine_limit=1).

For fully cached hot scans, it does seem to be quite sensitive to tiny
changes in a hot code path that I and others spent a lot of time
optimising and testing during the CF.  Perhaps it is possible that
someone else's microarchitecture or compiler could show a regression
that I don't see, and I will certainly look into it with vim and
vigour if so.  In that case we could consider a tiny
micro-optimisation that I've shared already (it seemed a little novel
so I'd rather propose it in the new cycle if I can), or, if it comes
to it based on evidence and inability to address a problem quickly,
reverting just b7b0f3f2 which itself is a very small patch.

An aspect you didn't mention is correctness.  I don't actually know
how to prove that buffer manager protocols are correct beyond thinking
and torture testing, ie what kind of new test harness machinery could
be used to cross-check more things about buffer pool state explicitly,
and that is a weakness I'm planning to look into.

I realise that "these are the good ones, you should see all the stuff
we decided not to commit!" is not an argument, I'm just laying out how
I see the patches that went in and why I thought they were good.  It's
almost an architectural change, but done in tiny footsteps.  I
appreciate that people would have liked to see those particular tiny
footsteps in some of the other fine months available for patching the
tree, and some of the earlier underpinning patches that were part of
the same patch series did go in around New Year, but clearly my
"commit spreading" didn't go as well as planned after that (not helped
by Jan/Feb summer vacation season down here).

Mr Paquier this year announced his personal code freeze a few weeks
back on social media, which seemed like an interesting idea I might
adopt.  Perhaps that is what some other people are doing without
saying so, and perhaps the time they are using for that is the end of
the calendar year.  I might still be naturally inclined to crunch-like
behaviour, but it wouldn't be at the same time as everyone else,
except all the people who follow the same advice.

On Tue, Apr 09, 2024 at 09:35:00AM +1200, Thomas Munro wrote:
> Mr Paquier this year announced his personal code freeze a few weeks
> back on social media, which seemed like an interesting idea I might
> adopt.  Perhaps that is what some other people are doing without
> saying so, and perhaps the time they are using for that is the end of
> the calendar year.  I might still be naturally inclined to crunch-like
> behaviour, but it wouldn't be at the same time as everyone else,
> except all the people who follow the same advice.

That's more linked to the fact that I was going silent without a
laptop for a few weeks before the end of the release cycle, and a way
to say to not count on me, while I was trying to keep my room clean to
avoid noise for others who would rush patches.  It is a vacation
period for schools in Japan as the fiscal year finishes at the end of
March, while the rest of the world still studies/works, so that makes
trips much easier with areas being less busy when going abroad.  If
you want to limit commit activity during this period, the answer is
simple then: require that all the committers live in Japan.

Jokes apart, I really try to split commit effort across the year and
not rush things at the last minute.  If something's not agreed upon
and commit-ready by the 15th of March, the chances that I would apply
it within the release cycle are really slim.  That's a kind of
personal policy I have in place for a few years now.
--
Michael

On 4/8/24 21:47, Robert Haas wrote:
> On Mon, Apr 8, 2024 at 10:42 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> Can you elaborate, which patches you think were not ready? Let's make
>> sure to capture any concrete concerns in the Open Items list.
> 
> ...
> 
> - Incremental backup could have all sorts of terrible data-corrupting
> bugs. 55a5ee30cd65886ff0a2e7ffef4ec2816fbec273 was a full brown-paper
> bag level fix, so the chances of there being further problems are
> probably high.
> 

I don't feel too particularly worried about this. Yes, backups are super
important because it's often the only thing you have left when things go
wrong, and the incremental aspect is all new. The code I've seen while
doing the CoW-related patches seemed very precise and careful, and the
one bug we found & fixed does not make it bad.

Sure, I can't rule there being more bugs, but I've been doing some
pretty extensive stress testing of this (doing incremental backups +
combinebackup, and comparing the results against the source, and that
sort of stuff). And so far only that single bug this way. I'm still
doing this randomized stress testing, with more and more complex
workloads etc. and I'll let keep doing that for a while.

Maybe I'm a bit too happy-go-lucky, but IMO the risk here is limited.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Apr 09, 2024 at 01:16:02AM +0200, Tomas Vondra wrote:
> I don't feel too particularly worried about this. Yes, backups are super
> important because it's often the only thing you have left when things go
> wrong, and the incremental aspect is all new. The code I've seen while
> doing the CoW-related patches seemed very precise and careful, and the
> one bug we found & fixed does not make it bad.
>
> Sure, I can't rule there being more bugs, but I've been doing some
> pretty extensive stress testing of this (doing incremental backups +
> combinebackup, and comparing the results against the source, and that
> sort of stuff). And so far only that single bug this way. I'm still
> doing this randomized stress testing, with more and more complex
> workloads etc. and I'll let keep doing that for a while.
>
> Maybe I'm a bit too happy-go-lucky, but IMO the risk here is limited.

Even if there's a critical bug, there are still other ways to take
backups, so there is an exit route even if a problem is found and even
if this problem requires a complex solution to be able to work
correctly.

This worries me less than other patches like the ones around heap
changes or the radix tree stuff for TID collection plugged into
vacuum, which don't have explicit on/off switches AFAIK.
--
Michael

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Apr 8, 2024 at 10:42 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> Can you elaborate, which patches you think were not ready? Let's make
>> sure to capture any concrete concerns in the Open Items list.

> Hi,

> I'm moving this topic to a new thread for better visibility and less
> admixture of concerns. I'd like to invite everyone here present to
> opine on which patches we ought to be worried about. Here are a few
> picks from me to start things off. My intention here is to convey "I
> find these scary" rather than "these commits were irresponsible," so I
> respectfully ask that you don't take the choice to list your patch
> here as an attack, or the choice not to list your patch here as an
> endorsement.

I have another one that I'm not terribly happy about:

    Author: Alexander Korotkov <akorotkov@postgresql.org>
    Branch: master [72bd38cc9] 2024-04-08 01:27:52 +0300

        Transform OR clauses to ANY expression

I don't know that I'd call it scary exactly, but I do think it
was premature.  A week ago there was no consensus that it was
ready to commit, but Alexander pushed it (or half of it, anyway)
despite that.  A few concrete concerns:

* Yet another planner GUC.  Do we really need or want that?

* What the medical community would call off-label usage of
query jumbling.  I'm not sure this is even correct as-used,
and for sure it's using that code for something never intended.
Nor is the added code adequately (as in, at all) documented.

* Patch refuses to group anything but Consts into the SAOP
transformation.  I realize that if you want to produce an
array Const you need Const inputs, but I wonder why it
wasn't considered to produce an ARRAY[] construct if there
are available clauses with pseudo-constant (eg Param)
comparison values.

* I really, really dislike jamming this logic into prepqual.c,
where it has no business being.  I note that it was shoved
into process_duplicate_ors without even the courtesy of
expanding the header comment:

 * process_duplicate_ors
 *      Given a list of exprs which are ORed together, try to apply
 *      the inverse OR distributive law.

Another reason to think this wasn't a very well chosen place is
that the file's list of #include's went from 4 entries to 11.
Somebody should have twigged to the idea that this was off-topic
for prepqual.c.

* OrClauseGroupKey is not a Node type, so why does it have
a NodeTag?  I wonder what value will appear in that field,
and what will happen if the struct is passed to any code
that expects real Nodes.

I could probably find some other nits if I spent more time
on it, but I think these are sufficient to show that this
was not commit-ready.

            regards, tom lane

On 9/4/2024 09:12, Tom Lane wrote:
> I have another one that I'm not terribly happy about:
> 
>      Author: Alexander Korotkov <akorotkov@postgresql.org>
>      Branch: master [72bd38cc9] 2024-04-08 01:27:52 +0300
> 
>          Transform OR clauses to ANY expression
Because I'm primary author of the idea, let me answer.
> 
> I don't know that I'd call it scary exactly, but I do think it
> was premature.  A week ago there was no consensus that it was
> ready to commit, but Alexander pushed it (or half of it, anyway)
> despite that.  A few concrete concerns:
> 
> * Yet another planner GUC.  Do we really need or want that?
It is the most interesting question here. Looking around planner 
features designed but not applied for the same reason because they can 
produce suboptimal plans in corner cases, I think about inventing 
flag-type parameters and hiding some features that work better for 
different load types under such flagged parameters.
> 
> * What the medical community would call off-label usage of
> query jumbling.  I'm not sure this is even correct as-used,
> and for sure it's using that code for something never intended.
> Nor is the added code adequately (as in, at all) documented.
I agree with documentation and disagree with critics on the expression 
jumbling. It was introduced in the core. Why don't we allow it to be 
used to speed up machinery with some hashing?
> 
> * Patch refuses to group anything but Consts into the SAOP
> transformation.  I realize that if you want to produce an
> array Const you need Const inputs, but I wonder why it
> wasn't considered to produce an ARRAY[] construct if there
> are available clauses with pseudo-constant (eg Param)
> comparison values.
Good point. I think, we can consider that in the future.
> 
> * I really, really dislike jamming this logic into prepqual.c,
> where it has no business being.  I note that it was shoved
> into process_duplicate_ors without even the courtesy of
> expanding the header comment:
Yeah, I preferred to do it in parse_expr.c with the assumption of some 
'minimal' or 'canonical' tree form. You can see this code in the 
previous version. I think we don't have any bugs here, but we have 
different opinions on how it should work.
> 
>   * process_duplicate_ors
>   *      Given a list of exprs which are ORed together, try to apply
>   *      the inverse OR distributive law.
> 
> Another reason to think this wasn't a very well chosen place is
> that the file's list of #include's went from 4 entries to 11.
> Somebody should have twigged to the idea that this was off-topic
> for prepqual.c.
> 
> * OrClauseGroupKey is not a Node type, so why does it have
> a NodeTag?  I wonder what value will appear in that field,
> and what will happen if the struct is passed to any code
> that expects real Nodes.
It's a hack authored by Alexander. I guess He can provide additional 
reasons in support of that.
> 
> I could probably find some other nits if I spent more time
> on it, but I think these are sufficient to show that this
> was not commit-ready.
It's up to you. On the one hand, I don't see any bugs or strong 
performance issues, and all the issues can be resolved further; on the 
other hand, I've got your good review and some ideas to work out.

-- 
regards,
Andrei Lepikhov
Postgres Professional

Andrei Lepikhov <a.lepikhov@postgrespro.ru> writes:
> On 9/4/2024 09:12, Tom Lane wrote:
>> I have another one that I'm not terribly happy about:
>> Author: Alexander Korotkov <akorotkov@postgresql.org>
>> Branch: master [72bd38cc9] 2024-04-08 01:27:52 +0300
>>     Transform OR clauses to ANY expression

>> * What the medical community would call off-label usage of
>> query jumbling.  I'm not sure this is even correct as-used,
>> and for sure it's using that code for something never intended.
>> Nor is the added code adequately (as in, at all) documented.

> I agree with documentation and disagree with critics on the expression 
> jumbling. It was introduced in the core. Why don't we allow it to be 
> used to speed up machinery with some hashing?

I would back up from that a good deal: why do we need to hash here in
the first place?  There's no evidence I'm aware of that it's needful
from a performance standpoint.

>> * I really, really dislike jamming this logic into prepqual.c,
>> where it has no business being.  I note that it was shoved
>> into process_duplicate_ors without even the courtesy of
>> expanding the header comment:

> Yeah, I preferred to do it in parse_expr.c with the assumption of some 
> 'minimal' or 'canonical' tree form.

That seems quite the wrong direction to me.  AFAICS, the argument
for making this transformation depends on being able to convert
to an indexscan condition, so I would try to apply it even later,
when we have a set of restriction conditions to apply to a particular
baserel.  (This would weaken the argument that we need hashing
rather than naive equal() tests even further, I think.)  Applying
the transform to join quals seems unlikely to be a win.

            regards, tom lane

On 09.04.24 00:58, Michael Paquier wrote:
> That's more linked to the fact that I was going silent without a
> laptop for a few weeks before the end of the release cycle, and a way
> to say to not count on me, while I was trying to keep my room clean to
> avoid noise for others who would rush patches.  It is a vacation
> period for schools in Japan as the fiscal year finishes at the end of
> March, while the rest of the world still studies/works, so that makes
> trips much easier with areas being less busy when going abroad.  If
> you want to limit commit activity during this period, the answer is
> simple then: require that all the committers live in Japan.

Well, due to the Easter holiday being earlier this year, I adopted a 
similar approach: Go on vacation the last week of March and watch the 
rest from the pool. :)  So far I feel this was better for my well-being.

On 4/9/24 01:33, Michael Paquier wrote:
> On Tue, Apr 09, 2024 at 01:16:02AM +0200, Tomas Vondra wrote:
>> I don't feel too particularly worried about this. Yes, backups are super
>> important because it's often the only thing you have left when things go
>> wrong, and the incremental aspect is all new. The code I've seen while
>> doing the CoW-related patches seemed very precise and careful, and the
>> one bug we found & fixed does not make it bad.
>>
>> Sure, I can't rule there being more bugs, but I've been doing some
>> pretty extensive stress testing of this (doing incremental backups +
>> combinebackup, and comparing the results against the source, and that
>> sort of stuff). And so far only that single bug this way. I'm still
>> doing this randomized stress testing, with more and more complex
>> workloads etc. and I'll let keep doing that for a while.
>>
>> Maybe I'm a bit too happy-go-lucky, but IMO the risk here is limited.
> 
> Even if there's a critical bug, there are still other ways to take
> backups, so there is an exit route even if a problem is found and even
> if this problem requires a complex solution to be able to work
> correctly.
> 

I think it's a bit more nuanced, because it's about backups/restore. The
bug might be subtle, and you won't learn about it until the moment when
you need to restore (or perhaps even long after that). At which point
"You might have taken the backup in some other way." is not really a
viable exit route.

Anyway, I'm still not worried about this particular feature, and I'll
keep doing the stress testing.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Apr 9, 2024 at 7:24 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
> I think it's a bit more nuanced, because it's about backups/restore. The
> bug might be subtle, and you won't learn about it until the moment when
> you need to restore (or perhaps even long after that). At which point
> "You might have taken the backup in some other way." is not really a
> viable exit route.
>
> Anyway, I'm still not worried about this particular feature, and I'll
> keep doing the stress testing.

In all sincerity, I appreciate the endorsement. Basically what's been
scaring me about this feature is the possibility that there's some
incurable design flaw that I've managed to completely miss. If it has
some more garden-variety bugs, that's still pretty bad: people will
potentially lose data and be unable to get it back. But, as long as
we're able to find the bugs and fix them, the situation should improve
over time until, hopefully, everybody trusts it roughly as much as we
trust, say, crash recovery. Perhaps even a bit more: I think this code
is much better-written than our crash recovery code, which has grown
into a giant snarl that nobody seems able to untangle, despite
multiple refactoring attempts. However, if there's some reason why the
approach is fundamentally unsound which I and others have failed to
detect, then we're at risk of shipping a feature that is irretrievably
broken. That would really suck.

I'm fairly hopeful that there is no such design defect: I certainly
can't think of one. But, it's much easier to imagine an incurable
problem here than with, say, the recent pruning+freezing changes.
Those changes might have bugs, and those bugs might be hard to find,
but if they do exist and are found, they can be fixed. Here, it's a
little less obvious that that's true. We're relying on our ability, at
incremental backup time, to sort out from the manifest and the WAL
summaries, what needs to be included in the backup in order for a
subsequent pg_combinebackup operation to produce correct results. The
basic idea is simple enough, but the details are complicated, and it
feels like a subtle defect in the algorithm could potentially scuttle
the whole thing. I'd certainly appreciate having more smart people try
to think of things that I might have overlooked.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 9/4/2024 12:55, Tom Lane wrote:
> Andrei Lepikhov <a.lepikhov@postgrespro.ru> writes:
>>> * I really, really dislike jamming this logic into prepqual.c,
>>> where it has no business being.  I note that it was shoved
>>> into process_duplicate_ors without even the courtesy of
>>> expanding the header comment:
> 
>> Yeah, I preferred to do it in parse_expr.c with the assumption of some
>> 'minimal' or 'canonical' tree form.
> 
> That seems quite the wrong direction to me.  AFAICS, the argument
> for making this transformation depends on being able to convert
> to an indexscan condition, so I would try to apply it even later,
> when we have a set of restriction conditions to apply to a particular
> baserel.  (This would weaken the argument that we need hashing
> rather than naive equal() tests even further, I think.)  Applying
> the transform to join quals seems unlikely to be a win.
Our first prototype did this job right at the stage of index path 
creation. Unfortunately, this approach was too narrow and expensive.
The most problematic cases we encountered were from BitmapOr paths: if 
an incoming query has a significant number of OR clauses, the optimizer 
spends a lot of time generating these, in most cases, senseless paths 
(remember also memory allocated for that purpose). Imagine how much 
worse the situation becomes when we scale it with partitions.
Another issue we resolved with this transformation: shorter list of 
clauses speeds up planning and, sometimes, makes cardinality estimation 
more accurate.
Moreover, it helps even SeqScan: attempting to find a value in the 
hashed array is much faster than cycling a long-expression on each 
incoming tuple.

One more idea that I have set aside here is that the planner can utilize 
quick clause hashing:
 From time to time, in the mailing list, I see disputes on different 
approaches to expression transformation/simplification/grouping, and 
most of the time, it ends up with the problem of search complexity. 
Clause hash can be a way to solve this, can't it?

-- 
regards,
Andrei Lepikhov
Postgres Professional

Hi,

On Tuesday, April 9th, 2024 at 2:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> In all sincerity, I appreciate the endorsement. Basically what's been
> scaring me about this feature is the possibility that there's some
> incurable design flaw that I've managed to completely miss. If it has
> some more garden-variety bugs, that's still pretty bad: people will
> potentially lose data and be unable to get it back. But, as long as
> we're able to find the bugs and fix them, the situation should improve
> over time until, hopefully, everybody trusts it roughly as much as we
> trust, say, crash recovery. Perhaps even a bit more: I think this code
> is much better-written than our crash recovery code, which has grown
> into a giant snarl that nobody seems able to untangle, despite
> multiple refactoring attempts. However, if there's some reason why the
> approach is fundamentally unsound which I and others have failed to
> detect, then we're at risk of shipping a feature that is irretrievably
> broken. That would really suck.

IMHO it totally worth shipping such long-waited feature sooner than later.
Yes, it is a complex one, but you started advertising it since last January already, so people should already be able
toplay with it in Beta. 

And as you mentioned in your blog about the evergreen backup:

> But if you're anything like me, you'll already see that this arrangement
> has two serious weaknesses. First, if there are any data-corrupting bugs
> in pg_combinebackup or any of the server-side code that supports
> incremental backup, this approach could get you into big trouble.

At some point, the only way to really validate a backup is to actually try to restore it.
And if people get encouraged to do that faster thanks to incremental backups, they could detect potential issues
sooner.
Ultimately, users will still need their full backups and WAL archives.
If pg_combinebackup fails for any reason, the fix will be to perform the recovery from the full backup directly.
They still should be able to recover, just slower.

--
Stefan FERCOT
Data Egret (https://dataegret.com)

On 2024-Apr-09, Stefan Fercot wrote:

> At some point, the only way to really validate a backup is to actually try to restore it.
> And if people get encouraged to do that faster thanks to incremental backups, they could detect potential issues
sooner.
> Ultimately, users will still need their full backups and WAL archives.
> If pg_combinebackup fails for any reason, the fix will be to perform the recovery from the full backup directly.
> They still should be able to recover, just slower.

I completely agree that people should be testing the feature so that we
can fix any bugs as soon as possible.  However, if my understanding is
correct, restoring a full backup plus an incremental no longer needs the
intervening WAL up to the incremental.  Users wishing to save some disk
space might be tempted to delete that WAL.  If they do, and later it
turns out that the full+incremental cannot be restored for whatever
reason, they are in danger.

But you're right that if they don't delete that WAL, then the full is
restorable on its own.

Maybe we should explicitly advise users to not delete that WAL from
their archives, until pg_combinebackup is hammered a bit more.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"I must say, I am absolutely impressed with what pgsql's implementation of
VALUES allows me to do. It's kind of ridiculous how much "work" goes away in
my code.  Too bad I can't do this at work (Oracle 8/9)."       (Tom Allison)
           http://archives.postgresql.org/pgsql-general/2007-06/msg00016.php

> On 9 Apr 2024, at 18:45, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Maybe we should explicitly advise users to not delete that WAL from
> their archives, until pg_combinebackup is hammered a bit more.

As a backup tool maintainer, I always reference to out-of-the box Postgres tools as some bulletproof alternative.
I really would like to stick to this reputation and not discredit these tools.


Best regards, Andrey Borodin.

On Mon, Apr 8, 2024 at 10:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I don't know that I'd call it scary exactly, but I do think it
> was premature.  A week ago there was no consensus that it was
> ready to commit, but Alexander pushed it (or half of it, anyway)
> despite that.

Some of the most compelling cases for the transformation will involve
path keys. If the transformation enables the optimizer to build a
plain index scan (or index-only scan) with useful path keys, then that
might well lead to a far superior plan compared to what's possible
with BitmapOrs.

I understand that it'll still be possible to use OR expression
evaluation in such cases, without applying the transformation (via
filter quals), so in principle you don't need the transformation to
get an index scan that can (say) terminate the scan early due to the
presence of an "ORDER BY ... LIMIT n". But I suspect that that won't
work out much of the time, because the planner will believe (rightly
or wrongly) that the filter quals will incur too many heap page
accesses.

Another problem (at least as I see it) with the new
or_to_any_transform_limit GUC is that its design seems to have nothing
to say about the importance of these sorts of cases. Most of these
cases will only have 2 or 3 constants, just because that's what's most
common in general.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> On Mon, Apr 8, 2024 at 10:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't know that I'd call it scary exactly, but I do think it
>> was premature.  A week ago there was no consensus that it was
>> ready to commit, but Alexander pushed it (or half of it, anyway)
>> despite that.

> Some of the most compelling cases for the transformation will involve
> path keys. If the transformation enables the optimizer to build a
> plain index scan (or index-only scan) with useful path keys, then that
> might well lead to a far superior plan compared to what's possible
> with BitmapOrs.

I did not say it isn't a useful thing to have.  I said the patch
did not appear ready to go in.

> I understand that it'll still be possible to use OR expression
> evaluation in such cases, without applying the transformation (via
> filter quals), so in principle you don't need the transformation to
> get an index scan that can (say) terminate the scan early due to the
> presence of an "ORDER BY ... LIMIT n". But I suspect that that won't
> work out much of the time, because the planner will believe (rightly
> or wrongly) that the filter quals will incur too many heap page
> accesses.

That's probably related to the fact that we don't have a mechanism
for evaluating non-indexed quals against columns that are retrievable
from the index.  We really oughta work on getting that done.  But
I've been keeping a side eye on the work to unify plain and index-only
scans, because that's going to touch a lot of the same code so it
doesn't seem profitable to pursue those goals in parallel.

> Another problem (at least as I see it) with the new
> or_to_any_transform_limit GUC is that its design seems to have nothing
> to say about the importance of these sorts of cases. Most of these
> cases will only have 2 or 3 constants, just because that's what's most
> common in general.

Yeah, that's one of the reasons I'm dubious that the committed
patch was ready.

            regards, tom lane

On Tue, Apr 9, 2024 at 12:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Some of the most compelling cases for the transformation will involve
> > path keys. If the transformation enables the optimizer to build a
> > plain index scan (or index-only scan) with useful path keys, then that
> > might well lead to a far superior plan compared to what's possible
> > with BitmapOrs.
>
> I did not say it isn't a useful thing to have.  I said the patch
> did not appear ready to go in.

Didn't mean to suggest otherwise. I just wanted to hear your thoughts
on this aspect of these sorts of transformations.

> > I understand that it'll still be possible to use OR expression
> > evaluation in such cases, without applying the transformation (via
> > filter quals), so in principle you don't need the transformation to
> > get an index scan that can (say) terminate the scan early due to the
> > presence of an "ORDER BY ... LIMIT n". But I suspect that that won't
> > work out much of the time, because the planner will believe (rightly
> > or wrongly) that the filter quals will incur too many heap page
> > accesses.
>
> That's probably related to the fact that we don't have a mechanism
> for evaluating non-indexed quals against columns that are retrievable
> from the index.  We really oughta work on getting that done.

I agree that that is very important work, but I'm not sure that it
makes all that much difference here. Even if we had that improved
mechanism already, today, using index quals would still be strictly
better than expression evaluation. Index quals can allow nbtree to
skip over irrelevant parts of the index as the need arises, which is a
significant advantage in its own right.

ISTM that the planner should always prefer index quals over expression
evaluation, on general principle, even when there's no reason to think
it'll work out. At worst the executor has essentially the same
physical access patterns as the expression evaluation case. On the
other hand, providing nbtree with that context might end up being a
great deal faster.

--
Peter Geoghegan

On Tue, Apr 9, 2024 at 8:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andrei Lepikhov <a.lepikhov@postgrespro.ru> writes:
> > On 9/4/2024 09:12, Tom Lane wrote:
> >> I have another one that I'm not terribly happy about:
> >> Author: Alexander Korotkov <akorotkov@postgresql.org>
> >> Branch: master [72bd38cc9] 2024-04-08 01:27:52 +0300
> >>     Transform OR clauses to ANY expression
>
> >> * What the medical community would call off-label usage of
> >> query jumbling.  I'm not sure this is even correct as-used,
> >> and for sure it's using that code for something never intended.
> >> Nor is the added code adequately (as in, at all) documented.
>
> > I agree with documentation and disagree with critics on the expression
> > jumbling. It was introduced in the core. Why don't we allow it to be
> > used to speed up machinery with some hashing?
>
> I would back up from that a good deal: why do we need to hash here in
> the first place?  There's no evidence I'm aware of that it's needful
> from a performance standpoint.

I think the feature is aimed to deal with large OR lists.  I've seen a
significant degradation on 10000 or-clause-groups.  That might seem
like awfully a lot, but actually it's not unachievable in generated
queries.

Links.
1. https://www.postgresql.org/message-id/CAPpHfduJtO0s9E%3DSHUTzrCD88BH0eik0UNog1_q3XBF2wLmH6g%40mail.gmail.com

------
Regards,
Alexander Korotkov

On Tue, Apr 9, 2024 at 5:12 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> * OrClauseGroupKey is not a Node type, so why does it have
> a NodeTag?  I wonder what value will appear in that field,
> and what will happen if the struct is passed to any code
> that expects real Nodes.

I used that to put both not-subject-of-transform nodes together with
hash entries into the same list.  This is used to save the order of
clauses.  I think this is an important property, and I have already
expressed it in [1].  That could be achieved without adding NodeTag to
hash entries, but that would require a level of indirection.  It's not
passed to code that expects real Nodes, it doesn't go to anything
except lists.

Links.
1. https://www.postgresql.org/message-id/CAPpHfdutHt31sdt2rfU%3D4fsDMWxf6tvtnHARgCzLY2Tf21%2Bfgw%40mail.gmail.com

------
Regards,
Alexander Korotkov

On Tue, Apr 9, 2024 at 8:37 AM Andrei Lepikhov
<a.lepikhov@postgrespro.ru> wrote:
> On 9/4/2024 09:12, Tom Lane wrote:
> > I have another one that I'm not terribly happy about:
> >
> >      Author: Alexander Korotkov <akorotkov@postgresql.org>
> >      Branch: master [72bd38cc9] 2024-04-08 01:27:52 +0300
> >
> >          Transform OR clauses to ANY expression
> Because I'm primary author of the idea, let me answer.
> >
> > I don't know that I'd call it scary exactly, but I do think it
> > was premature.  A week ago there was no consensus that it was
> > ready to commit, but Alexander pushed it (or half of it, anyway)
> > despite that.  A few concrete concerns:
> >
> > * Yet another planner GUC.  Do we really need or want that?
> It is the most interesting question here. Looking around planner
> features designed but not applied for the same reason because they can
> produce suboptimal plans in corner cases, I think about inventing
> flag-type parameters and hiding some features that work better for
> different load types under such flagged parameters.

Yes, I have spotted this transformation could cause a bitmap scan plan
regressions in [1] and [2].  Fixing that required to treat ANY the
same as OR for bitmap scans.  Andrei implemented that in [3], but that
increases planning complexity and elimitates significant part of the
advantages of OR-to-ANY transformation.

Links.
1. https://www.postgresql.org/message-id/CAPpHfduJtO0s9E%3DSHUTzrCD88BH0eik0UNog1_q3XBF2wLmH6g%40mail.gmail.com
2. https://www.postgresql.org/message-id/CAPpHfdtSXxhdv3mLOLjEewGeXJ%2BFtfhjqodn1WWuq5JLsKx48g%40mail.gmail.com
3. https://www.postgresql.org/message-id/6d27d752-db0b-4cac-9843-6ba3dd7a1e94%40postgrespro.ru

------
Regards,
Alexander Korotkov

On Tue, Apr 9, 2024 at 7:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Peter Geoghegan <pg@bowt.ie> writes:
> > On Mon, Apr 8, 2024 at 10:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I don't know that I'd call it scary exactly, but I do think it
> >> was premature.  A week ago there was no consensus that it was
> >> ready to commit, but Alexander pushed it (or half of it, anyway)
> >> despite that.
>
> > Some of the most compelling cases for the transformation will involve
> > path keys. If the transformation enables the optimizer to build a
> > plain index scan (or index-only scan) with useful path keys, then that
> > might well lead to a far superior plan compared to what's possible
> > with BitmapOrs.
>
> I did not say it isn't a useful thing to have.  I said the patch
> did not appear ready to go in.
>
> > I understand that it'll still be possible to use OR expression
> > evaluation in such cases, without applying the transformation (via
> > filter quals), so in principle you don't need the transformation to
> > get an index scan that can (say) terminate the scan early due to the
> > presence of an "ORDER BY ... LIMIT n". But I suspect that that won't
> > work out much of the time, because the planner will believe (rightly
> > or wrongly) that the filter quals will incur too many heap page
> > accesses.
>
> That's probably related to the fact that we don't have a mechanism
> for evaluating non-indexed quals against columns that are retrievable
> from the index.  We really oughta work on getting that done.  But
> I've been keeping a side eye on the work to unify plain and index-only
> scans, because that's going to touch a lot of the same code so it
> doesn't seem profitable to pursue those goals in parallel.
>
> > Another problem (at least as I see it) with the new
> > or_to_any_transform_limit GUC is that its design seems to have nothing
> > to say about the importance of these sorts of cases. Most of these
> > cases will only have 2 or 3 constants, just because that's what's most
> > common in general.
>
> Yeah, that's one of the reasons I'm dubious that the committed
> patch was ready.

While inventing this GUC, I was thinking more about avoiding
regressions rather than about unleashing the full power of this
optimization.  But now I see that that wasn't good enough.  And it was
definitely hasty to commit to this shape.  I apologize for this.

Tom, I think you are way more experienced in this codebase than me.
And, probably more importantly, more experienced in making decisions
for planner development.  If you see some way forward to polish this
post-commit, Andrei and I are ready to work hard on this with you.  If
you don't see (or don't think that's good), let's revert this.

------
Regards,
Alexander Korotkov

Alexander Korotkov <aekorotkov@gmail.com> writes:
> On Tue, Apr 9, 2024 at 5:12 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * OrClauseGroupKey is not a Node type, so why does it have
>> a NodeTag?  I wonder what value will appear in that field,
>> and what will happen if the struct is passed to any code
>> that expects real Nodes.

> I used that to put both not-subject-of-transform nodes together with
> hash entries into the same list.  This is used to save the order of
> clauses.  I think this is an important property, and I have already
> expressed it in [1].

What exactly is the point of having a NodeTag in the struct though?
If you don't need it to be a valid Node, that seems pointless and
confusing.  We certainly have plenty of other lists that contain
plain structs without tags, so I don't buy that the List
infrastructure is making you do that.

            regards, tom lane

Alexander Korotkov <aekorotkov@gmail.com> writes:
> On Tue, Apr 9, 2024 at 7:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Yeah, that's one of the reasons I'm dubious that the committed
>> patch was ready.

> While inventing this GUC, I was thinking more about avoiding
> regressions rather than about unleashing the full power of this
> optimization.  But now I see that that wasn't good enough.  And it was
> definitely hasty to commit to this shape.  I apologize for this.

> Tom, I think you are way more experienced in this codebase than me.
> And, probably more importantly, more experienced in making decisions
> for planner development.  If you see some way forward to polish this
> post-commit, Andrei and I are ready to work hard on this with you.  If
> you don't see (or don't think that's good), let's revert this.

It wasn't ready to commit, and I think trying to fix it up post
feature freeze isn't appropriate project management.  Let's revert
it and work on it more in the v18 time frame.

            regards, tom lane

On Tue, Apr 9, 2024 at 11:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > On Tue, Apr 9, 2024 at 5:12 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> * OrClauseGroupKey is not a Node type, so why does it have
> >> a NodeTag?  I wonder what value will appear in that field,
> >> and what will happen if the struct is passed to any code
> >> that expects real Nodes.
>
> > I used that to put both not-subject-of-transform nodes together with
> > hash entries into the same list.  This is used to save the order of
> > clauses.  I think this is an important property, and I have already
> > expressed it in [1].
>
> What exactly is the point of having a NodeTag in the struct though?
> If you don't need it to be a valid Node, that seems pointless and
> confusing.  We certainly have plenty of other lists that contain
> plain structs without tags, so I don't buy that the List
> infrastructure is making you do that.

This code mixes Expr's and hash entries in the single list.  The point
of having a NodeTag in the struct is the ability to distinguish them
later.

------
Regards,
Alexander Korotkov

On Tue, Apr 9, 2024 at 11:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > On Tue, Apr 9, 2024 at 7:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Yeah, that's one of the reasons I'm dubious that the committed
> >> patch was ready.
>
> > While inventing this GUC, I was thinking more about avoiding
> > regressions rather than about unleashing the full power of this
> > optimization.  But now I see that that wasn't good enough.  And it was
> > definitely hasty to commit to this shape.  I apologize for this.
>
> > Tom, I think you are way more experienced in this codebase than me.
> > And, probably more importantly, more experienced in making decisions
> > for planner development.  If you see some way forward to polish this
> > post-commit, Andrei and I are ready to work hard on this with you.  If
> > you don't see (or don't think that's good), let's revert this.
>
> It wasn't ready to commit, and I think trying to fix it up post
> feature freeze isn't appropriate project management.  Let's revert
> it and work on it more in the v18 time frame.

Ok, let's do this.  I'd like to hear from you some directions for
further development of this patch if possible.

------
Regards,
Alexander Korotkov

Alexander Korotkov <aekorotkov@gmail.com> writes:
> On Tue, Apr 9, 2024 at 11:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What exactly is the point of having a NodeTag in the struct though?
>> If you don't need it to be a valid Node, that seems pointless and
>> confusing.  We certainly have plenty of other lists that contain
>> plain structs without tags, so I don't buy that the List
>> infrastructure is making you do that.

> This code mixes Expr's and hash entries in the single list.  The point
> of having a NodeTag in the struct is the ability to distinguish them
> later.

If you're doing that, it really really ought to be a proper Node.
If nothing else, that would aid debugging by allowing the list
to be pprint'ed.

            regards, tom lane

On 4/10/24 01:59, Andrey M. Borodin wrote:
> 
>> On 9 Apr 2024, at 18:45, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>>
>> Maybe we should explicitly advise users to not delete that WAL from
>> their archives, until pg_combinebackup is hammered a bit more.
> 
> As a backup tool maintainer, I always reference to out-of-the box Postgres tools as some bulletproof alternative.
> I really would like to stick to this reputation and not discredit these tools.

+1.

Even so, only keeping WAL for the last backup is a dangerous move in any 
case. Lots of things can happen to a backup (other than bugs in the 
software) so keeping WAL back to the last full (or for all backups) is 
always an excellent idea.

Regards,
-David

On Wed, Apr 10, 2024 at 09:29:38AM +1000, David Steele wrote:
> Even so, only keeping WAL for the last backup is a dangerous move in any
> case. Lots of things can happen to a backup (other than bugs in the
> software) so keeping WAL back to the last full (or for all backups) is
> always an excellent idea.

Yeah, that's an excellent practive, but is why I'm less worried for
this feature.  The docs at [1] caution about "not to remove earlier
backups if they might be needed when restoring later incremental
backups".  Like Alvaro said, should we insist a bit more about the WAL
retention part in this section of the docs, down to the last full
backup?

[1]: https://www.postgresql.org/docs/devel/continuous-archiving.html#BACKUP-INCREMENTAL-BACKUP
--
Michael

On Tue, Apr 9, 2024 at 2:47 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> - I'm slightly worried about the TID store work (ee1b30f12, 30e144287,
> 667e65aac35), perhaps for no reason. Actually, the results seem really
> impressive,

First, thanks for the complement. I actually suspect if we had this
years ago, it might never have occurred to anyone to go through the
trouble of adding parallel index cleanup.

In a funny way, it's almost too effective -- the claim is that m_w_m
over 1GB is perfectly usable, but I haven't been able to get anywere
near that via vacuum (we have indirectly, via bespoke dev code,
but...). I don't have easy access to hardware that can hold a table
big enough to do so -- vacuuming 1 billion records stays under 400MB.

> and a very quick look at some of the commits seemed kind
> of reassuring. But, like the changes to pruning and freezing, this is
> making some really fundamental changes to critical code. In this case,
> it's code that has evolved very little over the years and seems to
> have now evolved quite a lot.

True. I'd say that at a high level, storage and retrieval of TIDs is a
lot simpler conceptually than other aspects of vacuuming. The
low-level guts are now much more complex, but I'm confident it won't
just output a wrong answer. That aspect has been working for a long
time, and when it has broken during development, it fails very quickly
and obviously.

The more challenging aspects are less cut-and-dried, like memory
management, delegation of responsibility, how to expose locking (which
vacuum doesn't even use), readability/maintainability. Those are more
subjective, but it seems to have finally clicked into place in a way
that feels like the right trade-offs have been made. That's hand-wavy,
I realize.

The more recent follow-up commits are pieces that were discussed and
planned for earlier, but have had less review and were left out from
the initial commits since they're not essential to the functionality.
I judged that test coverage was enough to have confidence in them.

On Tue, Apr 9, 2024 at 6:33 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> This worries me less than other patches like the ones around heap
> changes or the radix tree stuff for TID collection plugged into
> vacuum, which don't have explicit on/off switches AFAIK.

Yes, there is no switch. Interestingly enough, the previous item array
ended up with its own escape hatch to tamp down its eager allocation,
autovacuum_work_mem. Echoing what I said to Robert, if we had the new
storage years ago, I doubt this GUC would ever have been proposed.

I'll also mention the array is still in the code base, but only in a
test module as a standard to test against. Hopefully that offers some
reassurance.

On Tue, Apr 9, 2024 at 1:59 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> On 09.04.24 00:58, Michael Paquier wrote:
> > That's more linked to the fact that I was going silent without a
> > laptop for a few weeks before the end of the release cycle, and a way
> > to say to not count on me, while I was trying to keep my room clean to
> > avoid noise for others who would rush patches.  It is a vacation
> > period for schools in Japan as the fiscal year finishes at the end of
> > March, while the rest of the world still studies/works, so that makes
> > trips much easier with areas being less busy when going abroad.  If
> > you want to limit commit activity during this period, the answer is
> > simple then: require that all the committers live in Japan.
>
> Well, due to the Easter holiday being earlier this year, I adopted a
> similar approach: Go on vacation the last week of March and watch the
> rest from the pool. :)  So far I feel this was better for my well-being.

I usually aim to have my major work for the release code complete by
approximately September and committed by December or January. Then if
it slips, I still have a chance of finishing before the freeze, and if
it doesn't, then I don't have to deal with the mad flurry of activity
at the end, and perhaps there's even time for some follow-up work
afterwards (as in the case of IB), or just time to review some other
patches.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Apr 8, 2024 at 10:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I have another one that I'm not terribly happy about:
>
>     Author: Alexander Korotkov <akorotkov@postgresql.org>
>     Branch: master [72bd38cc9] 2024-04-08 01:27:52 +0300
>
>         Transform OR clauses to ANY expression

I realize that this has been reverted now, but what's really
frustrating about this case is that I reviewed this patch before and
gave feedback similar to some of the feedback you gave, and it just
didn't matter, and the patch was committed anyway.

> I don't know that I'd call it scary exactly, but I do think it
> was premature.  A week ago there was no consensus that it was
> ready to commit, but Alexander pushed it (or half of it, anyway)
> despite that.  A few concrete concerns:
>
> * Yet another planner GUC.  Do we really need or want that?

IMHO, no, and I said so in
https://www.postgresql.org/message-id/CA%2BTgmob%3DebuCHFSw327b55DJzE3JtOuZ5owxob%2BMgErb4me_Ag%40mail.gmail.com

> * What the medical community would call off-label usage of
> query jumbling.  I'm not sure this is even correct as-used,
> and for sure it's using that code for something never intended.
> Nor is the added code adequately (as in, at all) documented.

And I raised this point here:
https://www.postgresql.org/message-id/CA%2BTgmoZCgP6FrBQEusn4yaWm02XU8OPeoEMk91q7PRBgwaAkFw%40mail.gmail.com

> * Patch refuses to group anything but Consts into the SAOP
> transformation.  I realize that if you want to produce an
> array Const you need Const inputs, but I wonder why it
> wasn't considered to produce an ARRAY[] construct if there
> are available clauses with pseudo-constant (eg Param)
> comparison values.
>
> * I really, really dislike jamming this logic into prepqual.c,
> where it has no business being.  I note that it was shoved
> into process_duplicate_ors without even the courtesy of
> expanding the header comment:
>
>  * process_duplicate_ors
>  *        Given a list of exprs which are ORed together, try to apply
>  *        the inverse OR distributive law.
>
> Another reason to think this wasn't a very well chosen place is
> that the file's list of #include's went from 4 entries to 11.
> Somebody should have twigged to the idea that this was off-topic
> for prepqual.c.

All of this seems like it might be related to my comments in the above
email about the transformation being done too early.

> * OrClauseGroupKey is not a Node type, so why does it have
> a NodeTag?  I wonder what value will appear in that field,
> and what will happen if the struct is passed to any code
> that expects real Nodes.

I don't think I raised this issue.

> I could probably find some other nits if I spent more time
> on it, but I think these are sufficient to show that this
> was not commit-ready.

Just imagine if someone had taken time to give similar feedback before
the commit.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 4/10/24 09:50, Michael Paquier wrote:
> On Wed, Apr 10, 2024 at 09:29:38AM +1000, David Steele wrote:
>> Even so, only keeping WAL for the last backup is a dangerous move in any
>> case. Lots of things can happen to a backup (other than bugs in the
>> software) so keeping WAL back to the last full (or for all backups) is
>> always an excellent idea.
> 
> Yeah, that's an excellent practive, but is why I'm less worried for
> this feature.  The docs at [1] caution about "not to remove earlier
> backups if they might be needed when restoring later incremental
> backups".  Like Alvaro said, should we insist a bit more about the WAL
> retention part in this section of the docs, down to the last full
> backup?

I think that would make sense in general. But if we are doing it because 
we lack confidence in the incremental backup feature maybe that's a sign 
that the feature should be released as experimental (or not released at 
all).

Regards,
-David

On 4/11/24 10:23, Tom Kincaid wrote:
> 
> The extensive Beta process we have can be used to build confidence we 
> need in a feature that has extensive review and currently has no known 
> issues or outstanding objections.

I did have objections, here [1] and here [2]. I think the complexity, 
space requirements, and likely performance issues involved in restores 
are going to be a real problem for users. Some of these can be addressed 
in future releases, but I can't escape the feeling that what we are 
releasing here is half-baked.

Also, there are outstanding issues here [3] and now here [4].

Regards,
-David

[1] 
https://www.postgresql.org/message-id/590e3017-da1f-4af6-9bf0-1679511ca7e5%40pgmasters.net
[2] 
https://www.postgresql.org/message-id/11b38a96-6ded-4668-b772-40f992132797%40pgmasters.net
[3] 

https://www.postgresql.org/message-id/flat/05fb32c9-18d8-4f72-9af3-f41576c33119%40pgmasters.net#bb04b896f0f0147c10cee944a1391c1e
[4] 
https://www.postgresql.org/message-id/flat/9badd24d-5bd9-4c35-ba85-4c38a2feb73e%40pgmasters.net

On 4/11/24 03:52, David Steele wrote:
> On 4/11/24 10:23, Tom Kincaid wrote:
>>
>> The extensive Beta process we have can be used to build confidence we
>> need in a feature that has extensive review and currently has no known
>> issues or outstanding objections.
> 
> I did have objections, here [1] and here [2]. I think the complexity,
> space requirements, and likely performance issues involved in restores
> are going to be a real problem for users. Some of these can be addressed
> in future releases, but I can't escape the feeling that what we are
> releasing here is half-baked.
> 

I haven't been part of those discussions, and that part of the thread is
a couple months old already, so I'll share my view here instead.

I do not think it's half-baked. I certainly agree there are limitations,
and there's all kinds of bells and whistles we could add, but I think
the fundamental infrastructure is corrent and a meaningful step forward.
Would I wish it to handle .tar for example? Sure I would. But I think
it's something we can add in the future - if we require all of this to
happen in a single release, it'll never happen.

FWIW that discussion also mentions stuff that I think the feature should
not do. In particular, I don't think the ambition was (or should be) to
make pg_basebackup into a stand-alone tool. I always saw pg_basebackup
more as an interface to "backup steps" correctly rather than a complete
backup solution that'd manage backup registry, retention, etc.

> Also, there are outstanding issues here [3] and now here [4].
> 

I agree with some of this, I'll respond in the threads.


regards
Tomas

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi!

I also worked with this patch and until your explanation I didn’t fully 
understand the reasons why it was wrong to have this implementation when 
removing duplicate OR expressions.

Thank you, now I understand it!

I agree with explanation of Andrei Lepikhov regarding the fact that 
there were difficulties in moving the patch to another place and
the explanation of Alexander Korotkov and Peter Geoghegan regarding the 
need to apply this transformation.

Let me just add that initially this patch tried to solve a problem where 
50,000 OR expressions were generated and
there was a problem running that query using a plan with BitmapScan. I 
wrote more about this here [0].

If the patch can be improved and you can tell me how, because I don’t 
quite understand how to do it yet, to be honest, then I’ll be happy to 
work on it too.

[0] 
https://www.postgresql.org/message-id/052172e4-6d75-8069-3179-26de339dca03%40postgrespro.ru

-- 
Regards,
Alena Rybakina
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 4/11/24 20:26, Tomas Vondra wrote:
> 
> On 4/11/24 03:52, David Steele wrote:
>> On 4/11/24 10:23, Tom Kincaid wrote:
>>>
>>> The extensive Beta process we have can be used to build confidence we
>>> need in a feature that has extensive review and currently has no known
>>> issues or outstanding objections.
>>
>> I did have objections, here [1] and here [2]. I think the complexity,
>> space requirements, and likely performance issues involved in restores
>> are going to be a real problem for users. Some of these can be addressed
>> in future releases, but I can't escape the feeling that what we are
>> releasing here is half-baked.
> 
> I haven't been part of those discussions, and that part of the thread is
> a couple months old already, so I'll share my view here instead.
> 
> I do not think it's half-baked. I certainly agree there are limitations,
> and there's all kinds of bells and whistles we could add, but I think
> the fundamental infrastructure is corrent and a meaningful step forward.
> Would I wish it to handle .tar for example? Sure I would. But I think
> it's something we can add in the future - if we require all of this to
> happen in a single release, it'll never happen.

Fair enough, but the current release is extremely limited and it would 
be best if that was well understood by users.

> FWIW that discussion also mentions stuff that I think the feature should
> not do. In particular, I don't think the ambition was (or should be) to
> make pg_basebackup into a stand-alone tool. I always saw pg_basebackup
> more as an interface to "backup steps" correctly rather than a complete
> backup solution that'd manage backup registry, retention, etc.

Right -- this is exactly my issue. pg_basebackup was never easy to use 
as a backup solution and this feature makes it significantly more 
complicated. Complicated enough that it would be extremely difficult for 
most users to utilize in a meaningful way.

But they'll try because it is a new pg_basebackup feature and they'll 
assume it is there to be used. Maybe it would be a good idea to make it 
clear in the documentation that significant tooling will be required to 
make it work.

Regards,
-David

On Thu, Apr 11, 2024 at 5:48 PM David Steele <david@pgmasters.net> wrote:
> But they'll try because it is a new pg_basebackup feature and they'll
> assume it is there to be used. Maybe it would be a good idea to make it
> clear in the documentation that significant tooling will be required to
> make it work.

I don't agree with that idea. LOTS of what we ship takes a significant
amount of effort to make it work. You may well need a connection
pooler. You may well need a failover manager which may or may not be
separate from your connection pooler. You need a backup tool. You need
a replication management tool which may or may not be separate from
your backup tool and may or may not be separate from your failover
tool. You probably need various out-of-core connections for the
programming languages you need. You may need a management tool, and
you probably need a monitoring tool. Some of the tools you might
choose to do all that stuff themselves have a whole bunch of complex
dependencies. It's a mess.

Now, if someone were to say that we ought to talk about these issues
in our documentation and maybe give people some ideas about how to get
started, I would likely be in favor of that, modulo the small
political problem that various people would want their solution to be
the canonical one to which everyone gets referred. But I think it's
wrong to pretend like this feature is somehow special, that it's
somehow more raw or unfinished than tons of other things. I actually
think it's significantly *better* than a lot of other things. If we
add a disclaimer to the documentation saying "hey, this new
incremental backup feature is half-finished garbage!", and meanwhile
the documentation still says "hey, you can use cp as your
archive_command," then we have completely lost our minds.

I also think that you're being more negative about this than the facts
justify. As I said to several colleagues today, I *fully* acknowledge
that you have a lot more practical experience in this area than I do,
and a bunch of good ideas. I was really pleased to see you talking
about how it would be good if these tools worked on tar files - and I
completely agree, and I hope that will happen, and I hope to help in
making that happen. I think there are a bunch of other problems too,
only some of which I can guess at. However, I think saying that this
feature is not realistically intended to be used by end-users or that
they will not be able to do so is over the top, and is actually kind
of insulting. There has been more enthusiasm for this feature on this
mailing list and elsewhere than I've gotten for anything I've
developed in years. And I don't think that's because all of the people
who have expressed enthusiasm are silly geese who don't understand how
terrible it is.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 4/12/24 12:15, Robert Haas wrote:
> On Thu, Apr 11, 2024 at 5:48 PM David Steele <david@pgmasters.net> wrote:
>> But they'll try because it is a new pg_basebackup feature and they'll
>> assume it is there to be used. Maybe it would be a good idea to make it
>> clear in the documentation that significant tooling will be required to
>> make it work.
> 
> I don't agree with that idea. LOTS of what we ship takes a significant
> amount of effort to make it work. You may well need a connection
> pooler. You may well need a failover manager which may or may not be
> separate from your connection pooler. You need a backup tool. You need
> a replication management tool which may or may not be separate from
> your backup tool and may or may not be separate from your failover
> tool. You probably need various out-of-core connections for the
> programming languages you need. You may need a management tool, and
> you probably need a monitoring tool. Some of the tools you might
> choose to do all that stuff themselves have a whole bunch of complex
> dependencies. It's a mess.

The difference here is you *can* use Postgres without a connection 
pooler (I have many times) or failover (if downtime is acceptable) but 
most people would agree that you really *need* backup.

The backup tool should be clear and easy to use or misery will 
inevitably result. pg_basebackup is difficult enough to use and automate 
because it has no notion of a repository, no expiration, and no WAL 
handling just to name a few things. Now there is an even more advanced 
feature that is even harder to use. So, no, I really don't think this 
feature is practically usable by the vast majority of end users.

> Now, if someone were to say that we ought to talk about these issues
> in our documentation and maybe give people some ideas about how to get
> started, I would likely be in favor of that, modulo the small
> political problem that various people would want their solution to be
> the canonical one to which everyone gets referred. But I think it's
> wrong to pretend like this feature is somehow special, that it's
> somehow more raw or unfinished than tons of other things. I actually
> think it's significantly *better* than a lot of other things. If we
> add a disclaimer to the documentation saying "hey, this new
> incremental backup feature is half-finished garbage!", and meanwhile
> the documentation still says "hey, you can use cp as your
> archive_command," then we have completely lost our minds.

Fair point on cp, but that just points to an overall lack in our 
documentation and built-in backup/recovery tools in general.

> I also think that you're being more negative about this than the facts
> justify. As I said to several colleagues today, I *fully* acknowledge
> that you have a lot more practical experience in this area than I do,
> and a bunch of good ideas. I was really pleased to see you talking
> about how it would be good if these tools worked on tar files - and I
> completely agree, and I hope that will happen, and I hope to help in
> making that happen. I think there are a bunch of other problems too,
> only some of which I can guess at. However, I think saying that this
> feature is not realistically intended to be used by end-users or that
> they will not be able to do so is over the top, and is actually kind
> of insulting. 

It is not meant to be insulting, but I still believe it to be true. 
After years of working with users on backup problems I think I have a 
pretty good bead on what the vast majority of admins are capable of 
and/or willing to do. Making this feature work is pretty high above that 
bar.

If the primary motivation is to provide a feature that can be integrated 
with third party tools, as Tomas suggests, then I guess usability is 
somewhat moot. But you are insisting that is not the case and I just 
don't see it that way.

> There has been more enthusiasm for this feature on this
> mailing list and elsewhere than I've gotten for anything I've
> developed in years. And I don't think that's because all of the people
> who have expressed enthusiasm are silly geese who don't understand how
> terrible it is.

No doubt there is enthusiasm. It's a great feature to have. In 
particular I think the WAL summarizer is cool. But I do think the 
shortcomings are significant and that will become very apparent when 
people start to implement. The last minute effort to add COW support is 
an indication of problems that people will see in the field.

Further, I do think some less that ideal design decisions were made. In 
particular, I think sidelining manifests, i.e. making them optional, is 
not a good choice. This has led directly to the issue we see in [1]. If 
we require a manifest to make an incremental backup, why make it 
optional for combine?

This same design decision has led us to have "marker files" for 
zero-length files and unchanged files, which just seems extremely 
wasteful when these could be noted in the manifest. There are good 
reasons for writing everything out in a full backup, but for an 
incremental that can only be reconstructed using our tool the manifest 
should be sufficient.

Maybe all of this can be improved in a future release, along with tar 
reading, but none of those potential future improvements help me to 
believe that this is a user-friendly feature in this release.

Regards,
-David

---

[1] 
https://www.postgresql.org/message-id/flat/9badd24d-5bd9-4c35-ba85-4c38a2feb73e%40pgmasters.net

On 4/11/24 20:26, Tomas Vondra wrote:
> On 4/11/24 03:52, David Steele wrote:
>> On 4/11/24 10:23, Tom Kincaid wrote:
>>>
>>> The extensive Beta process we have can be used to build confidence we
>>> need in a feature that has extensive review and currently has no known
>>> issues or outstanding objections.
>>
>> I did have objections, here [1] and here [2]. I think the complexity,
>> space requirements, and likely performance issues involved in restores
>> are going to be a real problem for users. Some of these can be addressed
>> in future releases, but I can't escape the feeling that what we are
>> releasing here is half-baked.
>>
> I do not think it's half-baked. I certainly agree there are limitations,
> and there's all kinds of bells and whistles we could add, but I think
> the fundamental infrastructure is corrent and a meaningful step forward.
> Would I wish it to handle .tar for example? Sure I would. But I think
> it's something we can add in the future - if we require all of this to
> happen in a single release, it'll never happen.

I'm not sure that I really buy this argument, anyway. It is not uncommon 
for significant features to spend years in development before they are 
committed. This feature went from first introduction to commit in just 
over six months. Obviously Robert had been working on it for a while, 
but for a feature this large six months is a sprint.

Regards,
-David

On 4/11/24 23:48, David Steele wrote:
> On 4/11/24 20:26, Tomas Vondra wrote:
>>
>> On 4/11/24 03:52, David Steele wrote:
>>> On 4/11/24 10:23, Tom Kincaid wrote:
>>>>
>>>> The extensive Beta process we have can be used to build confidence we
>>>> need in a feature that has extensive review and currently has no known
>>>> issues or outstanding objections.
>>>
>>> I did have objections, here [1] and here [2]. I think the complexity,
>>> space requirements, and likely performance issues involved in restores
>>> are going to be a real problem for users. Some of these can be addressed
>>> in future releases, but I can't escape the feeling that what we are
>>> releasing here is half-baked.
>>
>> I haven't been part of those discussions, and that part of the thread is
>> a couple months old already, so I'll share my view here instead.
>>
>> I do not think it's half-baked. I certainly agree there are limitations,
>> and there's all kinds of bells and whistles we could add, but I think
>> the fundamental infrastructure is corrent and a meaningful step forward.
>> Would I wish it to handle .tar for example? Sure I would. But I think
>> it's something we can add in the future - if we require all of this to
>> happen in a single release, it'll never happen.
> 
> Fair enough, but the current release is extremely limited and it would
> be best if that was well understood by users.
> 
>> FWIW that discussion also mentions stuff that I think the feature should
>> not do. In particular, I don't think the ambition was (or should be) to
>> make pg_basebackup into a stand-alone tool. I always saw pg_basebackup
>> more as an interface to "backup steps" correctly rather than a complete
>> backup solution that'd manage backup registry, retention, etc.
> 
> Right -- this is exactly my issue. pg_basebackup was never easy to use
> as a backup solution and this feature makes it significantly more
> complicated. Complicated enough that it would be extremely difficult for
> most users to utilize in a meaningful way.
> 

Perhaps, I agree we could/should try to do better job to do backups, no
argument there. But I still don't quite see why introducing such
infrastructure to "manage backups" should be up to the patch adding
incremental backups. I see it as something to build on top of
pg_basebackup/pg_combinebackup, not into those tools.

> But they'll try because it is a new pg_basebackup feature and they'll
> assume it is there to be used. Maybe it would be a good idea to make it
> clear in the documentation that significant tooling will be required to
> make it work.
> 

Sure, I'm not against making it clearer pg_combinebackup is not a
complete backup solution, and documenting the existing restrictions.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 4/12/24 08:42, David Steele wrote:
> On 4/11/24 20:26, Tomas Vondra wrote:
>> On 4/11/24 03:52, David Steele wrote:
>>> On 4/11/24 10:23, Tom Kincaid wrote:
>>>>
>>>> The extensive Beta process we have can be used to build confidence we
>>>> need in a feature that has extensive review and currently has no known
>>>> issues or outstanding objections.
>>>
>>> I did have objections, here [1] and here [2]. I think the complexity,
>>> space requirements, and likely performance issues involved in restores
>>> are going to be a real problem for users. Some of these can be addressed
>>> in future releases, but I can't escape the feeling that what we are
>>> releasing here is half-baked.
>>>
>> I do not think it's half-baked. I certainly agree there are limitations,
>> and there's all kinds of bells and whistles we could add, but I think
>> the fundamental infrastructure is corrent and a meaningful step forward.
>> Would I wish it to handle .tar for example? Sure I would. But I think
>> it's something we can add in the future - if we require all of this to
>> happen in a single release, it'll never happen.
> 
> I'm not sure that I really buy this argument, anyway. It is not uncommon
> for significant features to spend years in development before they are
> committed. This feature went from first introduction to commit in just
> over six months. Obviously Robert had been working on it for a while,
> but for a feature this large six months is a sprint.
> 

Sure, but it's also not uncommon for significant features to be
developed incrementally, over multiple releases, introducing the basic
infrastructure first, and then expanding the capabilities later. I'd
cite logical decoding/replication and parallel query as examples of this
approach.

It's possible there's some fundamental flaw in the WAL summarization?
Sure, I can't rule that out, although I find it unlikely. Could there be
bugs? Sure, that's possible, but that applies to all code.

But it seems to me all the comments are about the client side, not about
the infrastructure. Which is fair, I certainly agree it'd be nice to
handle more use cases with less effort, but I still think the patch is a
meaningful step forward.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Apr 10, 2024 at 5:27 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Mon, Apr 8, 2024 at 10:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I have another one that I'm not terribly happy about:
> >
> >     Author: Alexander Korotkov <akorotkov@postgresql.org>
> >     Branch: master [72bd38cc9] 2024-04-08 01:27:52 +0300
> >
> >         Transform OR clauses to ANY expression
>
> I realize that this has been reverted now, but what's really
> frustrating about this case is that I reviewed this patch before and
> gave feedback similar to some of the feedback you gave, and it just
> didn't matter, and the patch was committed anyway.
>
> > I don't know that I'd call it scary exactly, but I do think it
> > was premature.  A week ago there was no consensus that it was
> > ready to commit, but Alexander pushed it (or half of it, anyway)
> > despite that.  A few concrete concerns:
> >
> > * Yet another planner GUC.  Do we really need or want that?
>
> IMHO, no, and I said so in
> https://www.postgresql.org/message-id/CA%2BTgmob%3DebuCHFSw327b55DJzE3JtOuZ5owxob%2BMgErb4me_Ag%40mail.gmail.com
>
> > * What the medical community would call off-label usage of
> > query jumbling.  I'm not sure this is even correct as-used,
> > and for sure it's using that code for something never intended.
> > Nor is the added code adequately (as in, at all) documented.
>
> And I raised this point here:
> https://www.postgresql.org/message-id/CA%2BTgmoZCgP6FrBQEusn4yaWm02XU8OPeoEMk91q7PRBgwaAkFw%40mail.gmail.com
>
> > * Patch refuses to group anything but Consts into the SAOP
> > transformation.  I realize that if you want to produce an
> > array Const you need Const inputs, but I wonder why it
> > wasn't considered to produce an ARRAY[] construct if there
> > are available clauses with pseudo-constant (eg Param)
> > comparison values.
> >
> > * I really, really dislike jamming this logic into prepqual.c,
> > where it has no business being.  I note that it was shoved
> > into process_duplicate_ors without even the courtesy of
> > expanding the header comment:
> >
> >  * process_duplicate_ors
> >  *        Given a list of exprs which are ORed together, try to apply
> >  *        the inverse OR distributive law.
> >
> > Another reason to think this wasn't a very well chosen place is
> > that the file's list of #include's went from 4 entries to 11.
> > Somebody should have twigged to the idea that this was off-topic
> > for prepqual.c.
>
> All of this seems like it might be related to my comments in the above
> email about the transformation being done too early.
>
> > * OrClauseGroupKey is not a Node type, so why does it have
> > a NodeTag?  I wonder what value will appear in that field,
> > and what will happen if the struct is passed to any code
> > that expects real Nodes.
>
> I don't think I raised this issue.
>
> > I could probably find some other nits if I spent more time
> > on it, but I think these are sufficient to show that this
> > was not commit-ready.
>
> Just imagine if someone had taken time to give similar feedback before
> the commit.

FWIW, I made my conclusion that it isn't worth to commit stuff like
this without explicit consent from Tom.  As well as it isn't worth to
commit table AM changes without explicit consent from Andres.  And it
isn't worth it to postpone large features to the last CF (it's better
to postpone to the next release then).

------
Regards,
Alexander Korotkov

On 4/12/24 22:12, Tomas Vondra wrote:
> On 4/11/24 23:48, David Steele wrote:
>> On 4/11/24 20:26, Tomas Vondra wrote:
 >>
>>> FWIW that discussion also mentions stuff that I think the feature should
>>> not do. In particular, I don't think the ambition was (or should be) to
>>> make pg_basebackup into a stand-alone tool. I always saw pg_basebackup
>>> more as an interface to "backup steps" correctly rather than a complete
>>> backup solution that'd manage backup registry, retention, etc.
>>
>> Right -- this is exactly my issue. pg_basebackup was never easy to use
>> as a backup solution and this feature makes it significantly more
>> complicated. Complicated enough that it would be extremely difficult for
>> most users to utilize in a meaningful way.
> 
> Perhaps, I agree we could/should try to do better job to do backups, no
> argument there. But I still don't quite see why introducing such
> infrastructure to "manage backups" should be up to the patch adding
> incremental backups. I see it as something to build on top of
> pg_basebackup/pg_combinebackup, not into those tools.

I'm not saying that managing backups needs to be part of pg_basebackup, 
but I am saying without that it is not a complete backup solution. 
Therefore introducing advanced features that the user then has to figure 
out how to manage puts a large burden on them. Implementing 
pg_combinebackup inefficiently out of the gate just makes their life harder.

>> But they'll try because it is a new pg_basebackup feature and they'll
>> assume it is there to be used. Maybe it would be a good idea to make it
>> clear in the documentation that significant tooling will be required to
>> make it work.
> 
> Sure, I'm not against making it clearer pg_combinebackup is not a
> complete backup solution, and documenting the existing restrictions.

Let's do that then. I think it would make sense to add caveats to the 
pg_combinebackup docs including space requirements, being explicit about 
the format required (e.g. plain), and also possible mitigation with COW 
filesystems.

Regards,
-David

On 4/12/24 22:27, Tomas Vondra wrote:
> 
> 
> On 4/12/24 08:42, David Steele wrote:
>> On 4/11/24 20:26, Tomas Vondra wrote:
>>> On 4/11/24 03:52, David Steele wrote:
>>>> On 4/11/24 10:23, Tom Kincaid wrote:
>>>>>
>>>>> The extensive Beta process we have can be used to build confidence we
>>>>> need in a feature that has extensive review and currently has no known
>>>>> issues or outstanding objections.
>>>>
>>>> I did have objections, here [1] and here [2]. I think the complexity,
>>>> space requirements, and likely performance issues involved in restores
>>>> are going to be a real problem for users. Some of these can be addressed
>>>> in future releases, but I can't escape the feeling that what we are
>>>> releasing here is half-baked.
>>>>
>>> I do not think it's half-baked. I certainly agree there are limitations,
>>> and there's all kinds of bells and whistles we could add, but I think
>>> the fundamental infrastructure is corrent and a meaningful step forward.
>>> Would I wish it to handle .tar for example? Sure I would. But I think
>>> it's something we can add in the future - if we require all of this to
>>> happen in a single release, it'll never happen.
>>
>> I'm not sure that I really buy this argument, anyway. It is not uncommon
>> for significant features to spend years in development before they are
>> committed. This feature went from first introduction to commit in just
>> over six months. Obviously Robert had been working on it for a while,
>> but for a feature this large six months is a sprint.
>>
> 
> Sure, but it's also not uncommon for significant features to be
> developed incrementally, over multiple releases, introducing the basic
> infrastructure first, and then expanding the capabilities later. I'd
> cite logical decoding/replication and parallel query as examples of this
> approach.
> 
> It's possible there's some fundamental flaw in the WAL summarization?
> Sure, I can't rule that out, although I find it unlikely. Could there be
> bugs? Sure, that's possible, but that applies to all code.
> 
> But it seems to me all the comments are about the client side, not about
> the infrastructure. Which is fair, I certainly agree it'd be nice to
> handle more use cases with less effort, but I still think the patch is a
> meaningful step forward.

Yes, my comments are all about the client code. I like the 
implementation of the WAL summarizer a lot. I don't think there is a 
fundamental flaw in the design, either, but I wouldn't be surprised if 
there are bugs. That's life in software development biz.

Even for the summarizer, though, I do worry about the complexity of 
maintaining it over time. It seems like it would be very easy to 
introduce a bug and have it go unnoticed until it causes problems in the 
field. A lot of testing was done outside of the test suite for this 
feature and I'm not sure if we can rely on that focus with every release.

For me an incremental approach would be to introduce the WAL summarizer 
first. There are already plenty of projects that do page-level 
incremental (WAL-G, pg_probackup, pgBackRest) and could help shake out 
the bugs. Then introduce the client tools later when they are more 
robust. Or, release the client tools now but mark them as experimental 
or something so people know that changes are coming and they don't get 
blindsided by that in the next release. Or, at the very least, make the 
caveats very clear so users can make an informed choice.

Regards,
-David

On 4/13/24 01:03, David Steele wrote:
> On 4/12/24 22:12, Tomas Vondra wrote:
>> On 4/11/24 23:48, David Steele wrote:
>>> On 4/11/24 20:26, Tomas Vondra wrote:
>>>
>>>> FWIW that discussion also mentions stuff that I think the feature
>>>> should
>>>> not do. In particular, I don't think the ambition was (or should be) to
>>>> make pg_basebackup into a stand-alone tool. I always saw pg_basebackup
>>>> more as an interface to "backup steps" correctly rather than a complete
>>>> backup solution that'd manage backup registry, retention, etc.
>>>
>>> Right -- this is exactly my issue. pg_basebackup was never easy to use
>>> as a backup solution and this feature makes it significantly more
>>> complicated. Complicated enough that it would be extremely difficult for
>>> most users to utilize in a meaningful way.
>>
>> Perhaps, I agree we could/should try to do better job to do backups, no
>> argument there. But I still don't quite see why introducing such
>> infrastructure to "manage backups" should be up to the patch adding
>> incremental backups. I see it as something to build on top of
>> pg_basebackup/pg_combinebackup, not into those tools.
> 
> I'm not saying that managing backups needs to be part of pg_basebackup,
> but I am saying without that it is not a complete backup solution.
> Therefore introducing advanced features that the user then has to figure
> out how to manage puts a large burden on them. Implementing
> pg_combinebackup inefficiently out of the gate just makes their life
> harder.
> 

I agree with this in general, but I fail to see how it'd be the fault of
this patch. It merely extends what pg_basebackup did before, so if it's
not a complete solution now, it wasn't a complete solution before.

Sure, I 100% agree it'd be great to have a more efficient
pg_combinebackup with fewer restrictions. But if we make these
limitations clear, I think it's better to have this than having nothing.

>>> But they'll try because it is a new pg_basebackup feature and they'll
>>> assume it is there to be used. Maybe it would be a good idea to make it
>>> clear in the documentation that significant tooling will be required to
>>> make it work.
>>
>> Sure, I'm not against making it clearer pg_combinebackup is not a
>> complete backup solution, and documenting the existing restrictions.
> 
> Let's do that then. I think it would make sense to add caveats to the
> pg_combinebackup docs including space requirements, being explicit about
> the format required (e.g. plain), and also possible mitigation with COW
> filesystems.
> 

OK. I'll add this as an open item, for me and Robert.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 4/13/24 01:23, David Steele wrote:
> On 4/12/24 22:27, Tomas Vondra wrote:
>>
>>
>> On 4/12/24 08:42, David Steele wrote:
>>> On 4/11/24 20:26, Tomas Vondra wrote:
>>>> On 4/11/24 03:52, David Steele wrote:
>>>>> On 4/11/24 10:23, Tom Kincaid wrote:
>>>>>>
>>>>>> The extensive Beta process we have can be used to build confidence we
>>>>>> need in a feature that has extensive review and currently has no
>>>>>> known
>>>>>> issues or outstanding objections.
>>>>>
>>>>> I did have objections, here [1] and here [2]. I think the complexity,
>>>>> space requirements, and likely performance issues involved in restores
>>>>> are going to be a real problem for users. Some of these can be
>>>>> addressed
>>>>> in future releases, but I can't escape the feeling that what we are
>>>>> releasing here is half-baked.
>>>>>
>>>> I do not think it's half-baked. I certainly agree there are
>>>> limitations,
>>>> and there's all kinds of bells and whistles we could add, but I think
>>>> the fundamental infrastructure is corrent and a meaningful step
>>>> forward.
>>>> Would I wish it to handle .tar for example? Sure I would. But I think
>>>> it's something we can add in the future - if we require all of this to
>>>> happen in a single release, it'll never happen.
>>>
>>> I'm not sure that I really buy this argument, anyway. It is not uncommon
>>> for significant features to spend years in development before they are
>>> committed. This feature went from first introduction to commit in just
>>> over six months. Obviously Robert had been working on it for a while,
>>> but for a feature this large six months is a sprint.
>>>
>>
>> Sure, but it's also not uncommon for significant features to be
>> developed incrementally, over multiple releases, introducing the basic
>> infrastructure first, and then expanding the capabilities later. I'd
>> cite logical decoding/replication and parallel query as examples of this
>> approach.
>>
>> It's possible there's some fundamental flaw in the WAL summarization?
>> Sure, I can't rule that out, although I find it unlikely. Could there be
>> bugs? Sure, that's possible, but that applies to all code.
>>
>> But it seems to me all the comments are about the client side, not about
>> the infrastructure. Which is fair, I certainly agree it'd be nice to
>> handle more use cases with less effort, but I still think the patch is a
>> meaningful step forward.
> 
> Yes, my comments are all about the client code. I like the
> implementation of the WAL summarizer a lot. I don't think there is a
> fundamental flaw in the design, either, but I wouldn't be surprised if
> there are bugs. That's life in software development biz.
> 

Agreed.

> Even for the summarizer, though, I do worry about the complexity of
> maintaining it over time. It seems like it would be very easy to
> introduce a bug and have it go unnoticed until it causes problems in the
> field. A lot of testing was done outside of the test suite for this
> feature and I'm not sure if we can rely on that focus with every release.
> 

I'm not sure there's a simpler way to implement this. I haven't really
worked on that part (not until the CoW changes a couple weeks ago), but
I think Robert was very conscious of the complexity.

I don't think expect this code to change very often, but I agree it's
not great to rely on testing outside the regular regression test suite.
But I'm not sure how much more we can do, really - for example my
testing was very much "randomized stress testing" with a lot of data and
long runs, looking for unexpected stuff. That's not something we could
do in the usual regression tests, I think.

But if you have suggestions how to extend the testing ...

> For me an incremental approach would be to introduce the WAL summarizer
> first. There are already plenty of projects that do page-level
> incremental (WAL-G, pg_probackup, pgBackRest) and could help shake out
> the bugs. Then introduce the client tools later when they are more
> robust. Or, release the client tools now but mark them as experimental
> or something so people know that changes are coming and they don't get
> blindsided by that in the next release. Or, at the very least, make the
> caveats very clear so users can make an informed choice.
> 

I don't think introducing just the summarizer, without any client tools,
would really work. How would we even test the summarizer, for example?
If the only users of that code are external tools, we'd do only some
very rudimentary tests. But the more complex tests would happen in the
external tools, which means it wouldn't be covered by cfbot, buildfarm
and so on. Considering the external tools are likely a bit behind, It's
not clear to me how I would do the stress testing, for example.

IMHO we should aim to have in-tree clients when possible, even if some
external tools can do more advanced stuff etc.

This however reminds me my question is the summarizer provides the right
interface(s) for the external tools. One option is to do pg_basebackup
and then parse the incremental files, but is that suitable for the
external tools, or should there be a more convenient way?


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 4/13/24 21:02, Tomas Vondra wrote:
> On 4/13/24 01:23, David Steele wrote:
> 
>> Even for the summarizer, though, I do worry about the complexity of
>> maintaining it over time. It seems like it would be very easy to
>> introduce a bug and have it go unnoticed until it causes problems in the
>> field. A lot of testing was done outside of the test suite for this
>> feature and I'm not sure if we can rely on that focus with every release.
>>
> 
> I'm not sure there's a simpler way to implement this. I haven't really
> worked on that part (not until the CoW changes a couple weeks ago), but
> I think Robert was very conscious of the complexity.
> 
> I don't think expect this code to change very often, but I agree it's
> not great to rely on testing outside the regular regression test suite.
> But I'm not sure how much more we can do, really - for example my
> testing was very much "randomized stress testing" with a lot of data and
> long runs, looking for unexpected stuff. That's not something we could
> do in the usual regression tests, I think.
> 
> But if you have suggestions how to extend the testing ...

Doing stress testing in the regular test suite is obviously a problem 
due to runtime, but it would still be great to see tests for issues that 
were found during external stress testing.

For example, the issue you and Jakub found was fixed in 55a5ee30 but 
there is no accompanying test and no existing test was broken by the change.

>> For me an incremental approach would be to introduce the WAL summarizer
>> first. There are already plenty of projects that do page-level
>> incremental (WAL-G, pg_probackup, pgBackRest) and could help shake out
>> the bugs. Then introduce the client tools later when they are more
>> robust. Or, release the client tools now but mark them as experimental
>> or something so people know that changes are coming and they don't get
>> blindsided by that in the next release. Or, at the very least, make the
>> caveats very clear so users can make an informed choice.
>>
> 
> I don't think introducing just the summarizer, without any client tools,
> would really work. How would we even test the summarizer, for example?
> If the only users of that code are external tools, we'd do only some
> very rudimentary tests. But the more complex tests would happen in the
> external tools, which means it wouldn't be covered by cfbot, buildfarm
> and so on. Considering the external tools are likely a bit behind, It's
> not clear to me how I would do the stress testing, for example.
> 
> IMHO we should aim to have in-tree clients when possible, even if some
> external tools can do more advanced stuff etc.
> 
> This however reminds me my question is the summarizer provides the right
> interface(s) for the external tools. One option is to do pg_basebackup
> and then parse the incremental files, but is that suitable for the
> external tools, or should there be a more convenient way?

Running a pg_basebackup to get the incremental changes would not be at 
all satisfactory. Luckily there are the 
pg_wal_summary_contents()/pg_available_wal_summaries() functions, which 
seem to provide the required information. I have not played with them 
much but I think they will do the trick.

They are pretty awkward to work with since they are essentially 
time-series data but what you'd really want, I think, is the ability to 
get page changes for a particular relfileid/segment.

Regards,
-David

Hi,

On Saturday, April 13th, 2024 at 12:18 PM, Tomas Vondra wrote:
> On 4/13/24 01:03, David Steele wrote:
> > On 4/12/24 22:12, Tomas Vondra wrote:
> > > On 4/11/24 23:48, David Steele wrote:
> > > > On 4/11/24 20:26, Tomas Vondra wrote:
> > > >
> > > > > FWIW that discussion also mentions stuff that I think the feature
> > > > > should
> > > > > not do. In particular, I don't think the ambition was (or should be) to
> > > > > make pg_basebackup into a stand-alone tool. I always saw pg_basebackup
> > > > > more as an interface to "backup steps" correctly rather than a complete
> > > > > backup solution that'd manage backup registry, retention, etc.
> > > >
> > > > Right -- this is exactly my issue. pg_basebackup was never easy to use
> > > > as a backup solution and this feature makes it significantly more
> > > > complicated. Complicated enough that it would be extremely difficult for
> > > > most users to utilize in a meaningful way.

pg_basebackup has its own use-cases IMHO. It is very handy to simply take a copy of the running cluster, thanks to its
abilityto carry on the needed WAL segs without the user even needing to think about archive_command/pg_receivewal. And
forthis kind of tasks, the new incremental thing will not really be interesting and won't make things more complicated. 

I totally agree that we don't have any complete backup solution in core though. And adding more tools to the picture
(pg_basebackup,pg_combinebackup, pg_receivewal, pg_verifybackup,...) will increase the need of on-top orchestration.
Butthat's not new. And for people already having such orchestration, having the incremental feature will help. 

> > > Perhaps, I agree we could/should try to do better job to do backups, no
> > > argument there. But I still don't quite see why introducing such
> > > infrastructure to "manage backups" should be up to the patch adding
> > > incremental backups. I see it as something to build on top of
> > > pg_basebackup/pg_combinebackup, not into those tools.
> >
> > I'm not saying that managing backups needs to be part of pg_basebackup,
> > but I am saying without that it is not a complete backup solution.
> > Therefore introducing advanced features that the user then has to figure
> > out how to manage puts a large burden on them. Implementing
> > pg_combinebackup inefficiently out of the gate just makes their life
> > harder.
>
> I agree with this in general, but I fail to see how it'd be the fault of
> this patch. It merely extends what pg_basebackup did before, so if it's
> not a complete solution now, it wasn't a complete solution before.

+1. We can see it as a step to having a better backup solution in core for the future, but we definitely shouldn't rule
outthe fact that lots of people already developed such orchestration (home-made or relying to pgbarman, pgbackrest,
wal-g,...).IMHO, if we're trying to extend the in core features, we should also aim at giving more lights and features
forthose tools (like adding more fields to the backup functions,...). 

> > > Sure, I'm not against making it clearer pg_combinebackup is not a
> > > complete backup solution, and documenting the existing restrictions.
> >
> > Let's do that then. I think it would make sense to add caveats to the
> > pg_combinebackup docs including space requirements, being explicit about
> > the format required (e.g. plain), and also possible mitigation with COW
> > filesystems.
>
> OK. I'll add this as an open item, for me and Robert.

Thanks for this! It's probably not up to core docs to state all the steps that would be needed to develop a complete
backupsolution but documenting the links between the tools and mostly all the caveats (the "don't use INCREMENTAL.*
filenames",...)will help users not be caught off guard. And as I mentioned in [1], IMO the earlier we can catch a
potentialissue (wrong filename, missing file,...), the better for the user. 

Thank you all for working on this.
Kind Regards,
--
Stefan FERCOT
Data Egret (https://dataegret.com)

[1]
https://www.postgresql.org/message-id/vJnnuiaye5rNnCPN8h3xN1Y3lyUDESIgEQnR-Urat9_ld_fozShSJbEk8JDM_3K6BVt5HXT-CatWpSfEZkYVeymlrxKO2_kfKmVZNWyCuJc%3D%40protonmail.com

On Mon, 2024-04-08 at 15:47 -0400, Robert Haas wrote:
> - I couldn't understand why the "Operate
> XLogCtl->log{Write,Flush}Result with atomics" code was correct when I
> read it.

I reviewed ee1cbe806d. It followed a good process of discussion and
review. It was a bit close to the feature freeze for comfort, but did
not feel rushed, to me.

Additional eyes are always welcome. There are also some related commits
in that area like f3ff7bf83b, c9920a9068 and 91f2cae7a4.

> But, a spinlock
> protecting two variables together guarantees more than atomic access
> to each of those variables separately.

We maintain the invariant:

   XLogCtl->logFlushResult <= XLogCtl->logWriteResult

and the non-shared version:

   LogwrtResult.Flush <= LogwrtResult.Write

and that the requests don't fall behind the results:

   XLogCtl->LogwrtRqst.Write >= XLogCtl->logWriteResult &&
   XLogCtl->LogwrtRqst.Flush >= XLogCtl->logFlushResult

Are you concerned that:

  (a) you aren't convinced that we're maintaining the invariants
properly? or
  (b) you aren't convinced that the invariant is strong enough, and
that there are other variables that we aren't considering?

And if you think this is right after all, where could we add some
clarification?

Regards,
    Jeff Davis

On Tue, Apr 16, 2024 at 7:20 PM Jeff Davis <pgsql@j-davis.com> wrote:
> We maintain the invariant:
>
>    XLogCtl->logFlushResult <= XLogCtl->logWriteResult
>
> and the non-shared version:
>
>    LogwrtResult.Flush <= LogwrtResult.Write
>
> and that the requests don't fall behind the results:

I had missed the fact that this commit added a bunch of read and write
barriers; and I think that's the main reason why I was concerned. The
existence of those barriers, and the invariants they are intended to
maintain, makes me feel better about it.

Thanks for writing back about this!

--
Robert Haas
EDB: http://www.enterprisedb.com