public inbox for [email protected]  

help / color / mirror / Atom feed

pg_dump versus hash partitioning
36+ messages / 9 participants
[nested] [flat]

* pg_dump versus hash partitioning
@ 2023-02-01 16:17  Tom Lane <[email protected]>
  0 siblings, 1 reply; 36+ messages in thread

From: Tom Lane @ 2023-02-01 16:17 UTC (permalink / raw)
  To: [email protected]; +Cc: Andrew <[email protected]>

Over at [1] we have a complaint that dump-and-restore fails for
hash-partitioned tables if a partitioning column is an enum,
because the enum values are unlikely to receive the same OIDs
in the destination database as they had in the source, and the
hash codes are dependent on those OIDs.  So restore tries to
load rows into the wrong leaf tables, and it's all a mess.
The patch approach proposed at [1] doesn't really work, but
what does work is to use pg_dump's --load-via-partition-root
option, so that the tuple routing decisions are all re-made.

I'd initially proposed that we force --load-via-partition-root
if we notice that we have hash partitioning on an enum column.
But the more I thought about this, the more comparable problems
came to mind:

1. Hash partitioning on text columns will likely fail if the
destination uses a different encoding.

2. Hash partitioning on float columns will fail if you use
--extra-float-digits to round off the values.  And then
there's the fact that the behavior of strtod() might vary
across platforms.

3. Hash partitioning on floats is also endian-dependent,
and the same is likely true for some other types.

4. Anybody want to bet that complex types such as jsonb
are entirely free of similar hazards?  (Yes, somebody
thought it'd be a good idea to provide jsonb_hash.)

In general, we've never thought that hash values are
required to be consistent across platforms.

That was leading me to think that we should force
--load-via-partition-root for any hash-partitioned table,
just to pre-emptively avoid these problems.  But then
I remembered that

5. Range partitioning on text columns will likely fail if the
destination uses a different collation.

This is looking like a very large-caliber foot-gun, isn't it?
And remember that --load-via-partition-root acts at pg_dump
time, not restore.  If all you have is a dump file with no
opportunity to go back and get a new one, and it won't load
into your new server, you have got a nasty problem.

I don't think this is an acceptable degree of risk, considering
that the primary use-cases for pg_dump involve target systems
that aren't 100.00% identical to the source.

So here's what I think we should actually do: make
--load-via-partition-root the default.  We can provide a
switch to turn it off, for those who are brave or foolish
enough to risk that in the name of saving a few cycles,
but it ought to be the default.

Furthermore, I think we should make this happen in time for
next week's releases.  I can write the patch easily enough,
but we need a consensus PDQ that this is what to do.

Anyone want to bikeshed on the spelling of the new switch?
I'm contemplating "--load-via-partition-leaf" or perhaps
"--no-load-via-partition-root".

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/765e5968-6c39-470f-95bf-7b14e6b9a1c0%40app.fastmail.com






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-01 17:39  Robert Haas <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 1 reply; 36+ messages in thread

From: Robert Haas @ 2023-02-01 17:39 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: [email protected]; Andrew <[email protected]>

On Wed, Feb 1, 2023 at 11:18 AM Tom Lane <[email protected]> wrote:
> Over at [1] we have a complaint that dump-and-restore fails for
> hash-partitioned tables if a partitioning column is an enum,
> because the enum values are unlikely to receive the same OIDs
> in the destination database as they had in the source, and the
> hash codes are dependent on those OIDs.

It seems to me that this is the root of the problem. We can't expect
to hash on something that's not present in the dump file and have
anything work.

> So here's what I think we should actually do: make
> --load-via-partition-root the default.  We can provide a
> switch to turn it off, for those who are brave or foolish
> enough to risk that in the name of saving a few cycles,
> but it ought to be the default.
>
> Furthermore, I think we should make this happen in time for
> next week's releases.  I can write the patch easily enough,
> but we need a consensus PDQ that this is what to do.

This seems extremely precipitous to me and I'm against it. I like the
fact that we have --load-via-partition-root, but it is a bit of a
hack. You don't get a single copy into the partition root, you get one
per child table -- and those COPY statements are listed as data for
the partitions where the data lives now, not for the parent table. I
am not completely sure whether there is a scenario where that's an
issue, but it's certainly an oddity. Also, and I think pretty
significantly, using --load-via-partition-root forces you to pay the
overhead of rerouting every tuple to the target partition whether you
need it or not, which is potentially a large unnecessary expense. I
don't think we should just foist that kind of overhead onto everyone
in every situation for every data type because somebody had a problem
in a certain case.

And even if we do decide to do that at some point, I don't think it is
right at all to rush such a change out on a short time scale, with
little time to mull over consequences and alternative fixes. I think
that could easily hurt more people than it helps.

I think that not all of the cases that you list are of the same type.
Loading a dump under a different encoding or on a different endianness
are surely corner cases. They might come up for some people
occasionally, but they're not typical. In the case of endianness,
that's because little-Endian has pretty much taken over the world; in
the case of encoding, that's because converting data between encodings
is a real pain, and combining that with a database dump and restore is
likely to be very little fun. It's hard to argue that collation
changes fall into the same category: we know that they get changed all
the time, often silently. But none of us database geeks think that's a
good thing: just that it's a thing that we have to deal with.

The enum case seems completely different to me. That's not the result
of trying to migrate your data to another architecture or of the glibc
maintainers not believing in sorting working the same way on Tuesday
that it did on Monday. That's the result of the PostgreSQL project
hashing data in a way that is does not make any real sense for the
application at hand. Any hash function that we use for partitioning
has to work based on data that is preserved by the dump-and-restore
process. I would argue that the float case is not of the same kind:
yes, if you round your data off, then the values are going to hash
differently, but if you truncate your strings, those will hash
differently too. Duh. Intentionally changing the value is supposed to
change the hash code, that's kind of the point of hashing.

So I think we should be asking ourselves what we could do about the
enum case specifically, rather than resorting to a bazooka.

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






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-01 20:34  Tom Lane <[email protected]>
  parent: Robert Haas <[email protected]>
  0 siblings, 2 replies; 36+ messages in thread

From: Tom Lane @ 2023-02-01 20:34 UTC (permalink / raw)
  To: Robert Haas <[email protected]>; +Cc: [email protected]; Andrew <[email protected]>

Robert Haas <[email protected]> writes:
> ... I like the
> fact that we have --load-via-partition-root, but it is a bit of a
> hack. You don't get a single copy into the partition root, you get one
> per child table -- and those COPY statements are listed as data for
> the partitions where the data lives now, not for the parent table. I
> am not completely sure whether there is a scenario where that's an
> issue, but it's certainly an oddity.

I spent a bit more time thinking about that, and while I agree that
it's an oddity, I don't see that it matters in the case of hash
partitioning.  You would notice an issue if you tried to do a selective
restore of just one partition --- but under what circumstance would
that be a useful thing to do?  By definition, under hash partitioning
there is no user-meaningful difference between different partitions.
Moreover, in the case at hand you would get constraint failures without
--load-via-partition-root, or tuple routing failures with it,
so what's the difference?  (Unless you'd created all the partitions
to start with and were doing a selective restore of just one partition's
data, in which case the outcome is "fails" or "works" respectively.)

> Also, and I think pretty
> significantly, using --load-via-partition-root forces you to pay the
> overhead of rerouting every tuple to the target partition whether you
> need it or not, which is potentially a large unnecessary expense.

Oddly, I always thought that we prioritize correctness over speed.
I don't mind having an option that allows people to select a less-safe
way of doing this, but I do think it's unwise for less-safe to be the
default, especially when it's something you can't fix after the fact.

What do you think of "--load-via-partition-root=on/off/auto", where
auto means "not with hash partitions" or the like?  I'm still
uncomfortable about the collation aspect, but I'm willing to concede
that range partitioning is less likely to fail in this way than hash.

			regards, tom lane






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-01 21:12  Peter Geoghegan <[email protected]>
  parent: Tom Lane <[email protected]>
  1 sibling, 0 replies; 36+ messages in thread

From: Peter Geoghegan @ 2023-02-01 21:12 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; [email protected]; Andrew <[email protected]>

On Wed, Feb 1, 2023 at 12:34 PM Tom Lane <[email protected]> wrote:
> > Also, and I think pretty
> > significantly, using --load-via-partition-root forces you to pay the
> > overhead of rerouting every tuple to the target partition whether you
> > need it or not, which is potentially a large unnecessary expense.
>
> Oddly, I always thought that we prioritize correctness over speed.
> I don't mind having an option that allows people to select a less-safe
> way of doing this, but I do think it's unwise for less-safe to be the
> default, especially when it's something you can't fix after the fact.

+1

As you pointed out already, pg_dump's primary use-cases all involve
target systems that aren't identical to the source system. Anybody
using pg_dump is unlikely to be particularly concerned about
performance.

> What do you think of "--load-via-partition-root=on/off/auto", where
> auto means "not with hash partitions" or the like?  I'm still
> uncomfortable about the collation aspect, but I'm willing to concede
> that range partitioning is less likely to fail in this way than hash.

Currently, pg_dump ignores collation versions entirely, except when
run by pg_upgrade. So pg_dump already understands that sometimes it's
important that the collation behavior be completely identical when the
database is restored, and sometimes it's desirable to produce a dump
with any available "logically equivalent" collation. This is about the
high level requirements, which makes sense to me.

ISTM that range partitioning is missing a good high level model that
builds on that. What's really needed is a fully worked out abstraction
that recognizes how collations can be equivalent for some purposes,
but not other purposes. The indirection between "logical and physical
collations" is underdeveloped. There isn't even an official name for
that idea.

-- 
Peter Geoghegan






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-01 21:14  Robert Haas <[email protected]>
  parent: Tom Lane <[email protected]>
  1 sibling, 2 replies; 36+ messages in thread

From: Robert Haas @ 2023-02-01 21:14 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: [email protected]; Andrew <[email protected]>

On Wed, Feb 1, 2023 at 3:34 PM Tom Lane <[email protected]> wrote:
> I spent a bit more time thinking about that, and while I agree that
> it's an oddity, I don't see that it matters in the case of hash
> partitioning.  You would notice an issue if you tried to do a selective
> restore of just one partition --- but under what circumstance would
> that be a useful thing to do?  By definition, under hash partitioning
> there is no user-meaningful difference between different partitions.
> Moreover, in the case at hand you would get constraint failures without
> --load-via-partition-root, or tuple routing failures with it,
> so what's the difference?  (Unless you'd created all the partitions
> to start with and were doing a selective restore of just one partition's
> data, in which case the outcome is "fails" or "works" respectively.)

I guess I was worried that pg_dump's dependency ordering stuff might
do something weird in some case that I'm not smart enough to think up.

> > Also, and I think pretty
> > significantly, using --load-via-partition-root forces you to pay the
> > overhead of rerouting every tuple to the target partition whether you
> > need it or not, which is potentially a large unnecessary expense.
>
> Oddly, I always thought that we prioritize correctness over speed.

That's a bit rich.

It seems to me that the job of pg_dump is to produce a dump that, when
reloaded on another system, recreates the same database state. That
means that we end up with all of the same objects, each defined in the
same way, and that all of the tables end up with all the same contents
that they had before. Here, you'd like to argue that it's perfectly
fine if we instead insert some of the rows into different tables than
where they were on the original system. Under normal circumstances, of
course, we wouldn't consider any such thing, because then we would not
be faithfully replicating the database state, which would be
incorrect. But here you want to argue that it's OK to create a
different database state because trying to recreate the same one would
produce an error and the user might not like getting an error so let's
just do something else instead and not even bother telling them.

As you have quite rightly pointed out, the --load-via-partition-root
behavior is useful for working around a variety of unfortunate things
that can happen. If a user is willing to say that getting a row into
one partition of some table is just as good as getting it into another
partition of that same table and that you don't mind paying the cost
associated with that, then that is something that we can do for that
user. But just as we normally prioritize correctness over speed, so
also do we normally throw errors when things aren't right instead of
silently accepting bad input. The partitions in this scenario are
tables that have constraints. If a dump contains a row that doesn't
satisfy some constraint on the table into which it is being loaded,
that's an error. Keep in mind that there's no rule that a user can't
query a partition directly.

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






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-01 21:44  Peter Geoghegan <[email protected]>
  parent: Robert Haas <[email protected]>
  1 sibling, 1 reply; 36+ messages in thread

From: Peter Geoghegan @ 2023-02-01 21:44 UTC (permalink / raw)
  To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; [email protected]; Andrew <[email protected]>

On Wed, Feb 1, 2023 at 1:14 PM Robert Haas <[email protected]> wrote:
> It seems to me that the job of pg_dump is to produce a dump that, when
> reloaded on another system, recreates the same database state. That
> means that we end up with all of the same objects, each defined in the
> same way, and that all of the tables end up with all the same contents
> that they had before. Here, you'd like to argue that it's perfectly
> fine if we instead insert some of the rows into different tables than
> where they were on the original system. Under normal circumstances, of
> course, we wouldn't consider any such thing, because then we would not
> be faithfully replicating the database state, which would be
> incorrect. But here you want to argue that it's OK to create a
> different database state because trying to recreate the same one would
> produce an error and the user might not like getting an error so let's
> just do something else instead and not even bother telling them.

This is a misrepresentation of Tom's words. It isn't actually
self-evident what "we end up with all of the same objects, each
defined in the same way, and that all of the tables end up with all
the same contents that they had before" actually means here, in
general. Tom's main concern seems to be just that -- the ambiguity
itself.

If there was a fully worked out idea of what that would mean, then I
suspect it would be quite subtle and complicated -- it's an inherently
tricky area. You seem to be saying that the way that this stuff
currently works is correct by definition, except when it isn't.

-- 
Peter Geoghegan






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-01 22:08  Tom Lane <[email protected]>
  parent: Robert Haas <[email protected]>
  1 sibling, 1 reply; 36+ messages in thread

From: Tom Lane @ 2023-02-01 22:08 UTC (permalink / raw)
  To: Robert Haas <[email protected]>; +Cc: [email protected]; Andrew <[email protected]>

Robert Haas <[email protected]> writes:
> It seems to me that the job of pg_dump is to produce a dump that, when
> reloaded on another system, recreates the same database state. That
> means that we end up with all of the same objects, each defined in the
> same way, and that all of the tables end up with all the same contents
> that they had before.

No, its job is to produce *logically* the same state.  We don't expect
it to produce, say, the same CTID value that each row had before ---
if you want that, you use pg_upgrade or physical replication, not
pg_dump.  There are fairly strong constraints on how identical we
can make the results given that we're not replicating physical state.
And that's not even touching the point that frequently what the user
is after is not an identical copy anyway.

> Here, you'd like to argue that it's perfectly
> fine if we instead insert some of the rows into different tables than
> where they were on the original system.

I can agree with that argument for range or list partitioning, where
the partitions have some semantic meaning to the user.  I don't buy it
for hash partitioning.  It was an implementation artifact to begin
with that a given row ended up in partition 3 not partition 11, so why
would users care which partition it ends up in after a dump/reload?
If they think there is a difference between the partitions, they need
education.

> ... But just as we normally prioritize correctness over speed, so
> also do we normally throw errors when things aren't right instead of
> silently accepting bad input. The partitions in this scenario are
> tables that have constraints.

Again, for hash partitioning those constraints are implementation
artifacts not something that users should have any concern with.

> Keep in mind that there's no rule that a user can't
> query a partition directly.

Sure, and in the case of a hash partition, what he is going to
get is an implementation-dependent subset of the rows.  I use
"implementation-dependent" here in the same sense that the SQL
standard does, which is "there is a rule but the implementation
doesn't have to tell you what it is".  In particular, we are not
bound to make the subset be the same on different installations.
We already didn't promise that, because of issues like endianness.

			regards, tom lane






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-01 22:12  Robert Haas <[email protected]>
  parent: Peter Geoghegan <[email protected]>
  0 siblings, 2 replies; 36+ messages in thread

From: Robert Haas @ 2023-02-01 22:12 UTC (permalink / raw)
  To: Peter Geoghegan <[email protected]>; +Cc: Tom Lane <[email protected]>; [email protected]; Andrew <[email protected]>

On Wed, Feb 1, 2023 at 4:44 PM Peter Geoghegan <[email protected]> wrote:
> This is a misrepresentation of Tom's words. It isn't actually
> self-evident what "we end up with all of the same objects, each
> defined in the same way, and that all of the tables end up with all
> the same contents that they had before" actually means here, in
> general. Tom's main concern seems to be just that -- the ambiguity
> itself.

As far as I can see, Tom didn't admit that there was any ambiguity. He
just said that I was advocating for wrong behavior for the sake of
performance. I don't think that is what I was doing. I also don't
really think Tom thinks that that is what I was doing. But it is what
he said I was doing.

I do agree with you that the ambiguity is the root of the issue. I
mean, if we can't put the rows back into the same partitions where
they were before, does the user care about that, or do they only care
that the rows end up in some partition of the toplevel partitioned
table? I think that there's no single definition of correctness that
is the only defensible one here, and we can't know which one the user
wants a priori. I also think that they can care which one they're
getting, and thus I think that changing the default in a minor release
is a bad plan. Tom, as I understand it, is arguing that the
--load-via-partition-root behavior has negligible downsides and is
almost categorically better than the current default behavior, and
thus making that the new default in some or all situations in a minor
release is totally fine.

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






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-01 22:33  Tom Lane <[email protected]>
  parent: Robert Haas <[email protected]>
  1 sibling, 0 replies; 36+ messages in thread

From: Tom Lane @ 2023-02-01 22:33 UTC (permalink / raw)
  To: Robert Haas <[email protected]>; +Cc: Peter Geoghegan <[email protected]>; [email protected]; Andrew <[email protected]>

Robert Haas <[email protected]> writes:
> Tom, as I understand it, is arguing that the
> --load-via-partition-root behavior has negligible downsides and is
> almost categorically better than the current default behavior, and
> thus making that the new default in some or all situations in a minor
> release is totally fine.

I think it's categorically better than a failed restore.  I wouldn't
be proposing this if there were no such problem; but there is,
and I don't buy your apparent position that we should leave affected
users to cope as best they can.  Yes, it's clearly only a minority
of users that are affected, else we'd have heard complaints before.
But it could be absolutely catastrophic for an affected user,
if they're trying to restore their only backup.  I'd rather impose
an across-the-board cost on all users of hash partitioning than
risk such outcomes for a few.

Also, you've really offered no evidence for your apparent position
that --load-via-partition-root has unacceptable overhead.  We've
done enough work on partition routing over the last few years that
whatever measurements might've originally justified that idea
don't necessarily apply anymore.  Admittedly, I've not measured
it either.  But we don't tell people to avoid partitioning because
INSERT is unduly expensive.  Partition routing is just the cost of
doing business in that space.

			regards, tom lane






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-01 22:33  Peter Geoghegan <[email protected]>
  parent: Robert Haas <[email protected]>
  1 sibling, 1 reply; 36+ messages in thread

From: Peter Geoghegan @ 2023-02-01 22:33 UTC (permalink / raw)
  To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; [email protected]; Andrew <[email protected]>

On Wed, Feb 1, 2023 at 2:12 PM Robert Haas <[email protected]> wrote:
> On Wed, Feb 1, 2023 at 4:44 PM Peter Geoghegan <[email protected]> wrote:
> > This is a misrepresentation of Tom's words. It isn't actually
> > self-evident what "we end up with all of the same objects, each
> > defined in the same way, and that all of the tables end up with all
> > the same contents that they had before" actually means here, in
> > general. Tom's main concern seems to be just that -- the ambiguity
> > itself.
>
> As far as I can see, Tom didn't admit that there was any ambiguity.

Tom said:

"I spent a bit more time thinking about that, and while I agree that
it's an oddity, I don't see that it matters in the case of hash
partitioning.  You would notice an issue if you tried to do a
selective restore of just one partition --- but under what
circumstance would that be a useful thing to do?"

While the word ambiguity may not have actually been used, Tom very
clearly admitted some ambiguity. But even if he didn't, so what? It's
perfectly obvious that that's the major underlying issue, and that
this is a high level problem rather than a low level problem.

> He just said that I was advocating for wrong behavior for the sake of
> performance. I don't think that is what I was doing. I also don't
> really think Tom thinks that that is what I was doing. But it is what
> he said I was doing.

And I think that you're making a mountain out of a molehill.

> I do agree with you that the ambiguity is the root of the issue. I
> mean, if we can't put the rows back into the same partitions where
> they were before, does the user care about that, or do they only care
> that the rows end up in some partition of the toplevel partitioned
> table?

That was precisely the question that Tom posed to you, in the same
email as the one that you found objectionable.

> Tom, as I understand it, is arguing that the
> --load-via-partition-root behavior has negligible downsides and is
> almost categorically better than the current default behavior, and
> thus making that the new default in some or all situations in a minor
> release is totally fine.

I don't know why you seem to think that it was such an absolutist
position as that.

You mentioned "minor releases" here. Who said anything about that?

-- 
Peter Geoghegan






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-01 22:36  Robert Haas <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 2 replies; 36+ messages in thread

From: Robert Haas @ 2023-02-01 22:36 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: [email protected]; Andrew <[email protected]>

On Wed, Feb 1, 2023 at 5:08 PM Tom Lane <[email protected]> wrote:
> > Here, you'd like to argue that it's perfectly
> > fine if we instead insert some of the rows into different tables than
> > where they were on the original system.
>
> I can agree with that argument for range or list partitioning, where
> the partitions have some semantic meaning to the user.  I don't buy it
> for hash partitioning.  It was an implementation artifact to begin
> with that a given row ended up in partition 3 not partition 11, so why
> would users care which partition it ends up in after a dump/reload?
> If they think there is a difference between the partitions, they need
> education.

I see your point. I think it's somewhat valid. However, I also think
it muddies the definition of what pg_dump is allowed to do in a way
that I do not like. I think there's a difference between the CTID or
XMAX of a tuple changing and it ending up in a totally different
partition. It feels like it makes the definition of correctness
subjective: we do think that people care about range and list
partitions as individual entities, so we'll put the rows back where
they were and complain if we can't, but we don't think they think
about hash partitions that way, so we will err on the side of making
the dump restoration succeed. That's a level of guessing what the user
meant that I think is uncomfortable. I feel like when somebody around
here discovers that sort of reasoning in some other software's code,
or in a proposed patch, it pretty often gets blasted on this mailing
list with considerable vigor.

I think you can construct plausible cases where it's not just
academic. For instance, suppose I intend to use some kind of logical
replication system, not necessarily the one built into PostgreSQL, to
replicate data between two systems. Before engaging that system, I
need to make the initial database contents match. The system is
oblivious to partitioning, and just replicates each table to a table
with a matching name. Well, if the partitions don't actually match up
1:1, I kind of need to know about that. In this use case, the rows
silently moving around doesn't meet my needs. Or, suppose I dump and
restore two databases. It works perfectly. I then run a comparison
tool of some sort that compares the two databases. EDB has such a
tool! I don't know whether it would perform the comparison via the
partition root or not, because I don't know how it works. But I find
it pretty plausible that some such tool would show differences between
the source and target databases. Now, if I had done the data migration
using --load-with-partition-root, I would expect that. I might even be
looking for it, to see what got moved around. But otherwise, it might
be unexpected.

Another subtle problem with this whole situation is: suppose that on
host A, I set up a table hash-partitioned by an enum column and make a
bunch of hash partitions. Then, on host B, I set up the same table
with a bunch of foreign table partitions, each corresponding to the
matching partition on the other node. I guess that just doesn't work,
whereas if the column were of any other data type, it would work. If
it were a collatable data type, it would need the collations and
collation definitions to match, too.

I know these things are subtle, and maybe these specific things will
never happen to anyone, or nobody will care. I don't know. I just have
a really hard time accepting that a categorical statement that this
just can't ever matter to anyone.

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






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-01 22:38  Tom Lane <[email protected]>
  parent: Peter Geoghegan <[email protected]>
  0 siblings, 1 reply; 36+ messages in thread

From: Tom Lane @ 2023-02-01 22:38 UTC (permalink / raw)
  To: Peter Geoghegan <[email protected]>; +Cc: Robert Haas <[email protected]>; [email protected]; Andrew <[email protected]>

Peter Geoghegan <[email protected]> writes:
> You mentioned "minor releases" here. Who said anything about that?

I did: I'd like to back-patch the fix if possible.  I think changing
the default --load-via-partition-root choice could be back-patchable.

If Robert is resistant to that but would accept it in master,
I'd settle for that in preference to having no fix.

			regards, tom lane






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-01 22:49  Tom Lane <[email protected]>
  parent: Robert Haas <[email protected]>
  1 sibling, 2 replies; 36+ messages in thread

From: Tom Lane @ 2023-02-01 22:49 UTC (permalink / raw)
  To: Robert Haas <[email protected]>; +Cc: [email protected]; Andrew <[email protected]>

Robert Haas <[email protected]> writes:
> On Wed, Feb 1, 2023 at 5:08 PM Tom Lane <[email protected]> wrote:
>> I can agree with that argument for range or list partitioning, where
>> the partitions have some semantic meaning to the user.  I don't buy it
>> for hash partitioning.  It was an implementation artifact to begin
>> with that a given row ended up in partition 3 not partition 11, so why
>> would users care which partition it ends up in after a dump/reload?
>> If they think there is a difference between the partitions, they need
>> education.

> I see your point. I think it's somewhat valid. However, I also think
> it muddies the definition of what pg_dump is allowed to do in a way
> that I do not like. I think there's a difference between the CTID or
> XMAX of a tuple changing and it ending up in a totally different
> partition. It feels like it makes the definition of correctness
> subjective: we do think that people care about range and list
> partitions as individual entities, so we'll put the rows back where
> they were and complain if we can't, but we don't think they think
> about hash partitions that way, so we will err on the side of making
> the dump restoration succeed. That's a level of guessing what the user
> meant that I think is uncomfortable.

I see your point too, and to me it's evidence for the position that
we should never have done hash partitioning in the first place.
It's precisely because you want to analyze it in the same terms
as range/list partitioning that we have these issues.  Or we could
have built it on some other infrastructure than hash index opclasses
... but we didn't do that, and now we have a mess.  I don't see a way
out other than relaxing the guarantees about how hash partitioning
works compared to the other kinds.

			regards, tom lane






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-01 23:15  Peter Geoghegan <[email protected]>
  parent: Tom Lane <[email protected]>
  1 sibling, 0 replies; 36+ messages in thread

From: Peter Geoghegan @ 2023-02-01 23:15 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; [email protected]; Andrew <[email protected]>

On Wed, Feb 1, 2023 at 2:49 PM Tom Lane <[email protected]> wrote:
> It's precisely because you want to analyze it in the same terms
> as range/list partitioning that we have these issues.  Or we could
> have built it on some other infrastructure than hash index opclasses
> ... but we didn't do that, and now we have a mess.  I don't see a way
> out other than relaxing the guarantees about how hash partitioning
> works compared to the other kinds.

I suspect that using hash index opclasses was the right design --
sticking to the same approach to hashing seems valuable. I agree with
your overall conclusion, though, since it doesn't seem sensible to
allow hashing behavior to ever be anything greater than an
implementation detail. On general principle. What happens when a hash
function is discovered to have a huge flaw, as happened a couple of
times before now?

It's just the same with collations, where a particular collation
shouldn't be expected to have perfectly stable behavior across a dump
and reload. While admitting that possibility does open the door to
problems, in particular problems when range partitioning is in use,
those problems at least make sense. And they probably won't come up
very often -- collation updates don't often contain enormous
gratuitous differences that are liable to create dump/reload hazards
with range partitioning. It is the least worst approach, overall. In
theory, and in practice.

-- 
Peter Geoghegan






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-02 00:15  David Rowley <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 1 reply; 36+ messages in thread

From: David Rowley @ 2023-02-02 00:15 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Peter Geoghegan <[email protected]>; Robert Haas <[email protected]>; [email protected]; Andrew <[email protected]>

On Thu, 2 Feb 2023 at 11:38, Tom Lane <[email protected]> wrote:
>
> Peter Geoghegan <[email protected]> writes:
> > You mentioned "minor releases" here. Who said anything about that?
>
> I did: I'd like to back-patch the fix if possible.  I think changing
> the default --load-via-partition-root choice could be back-patchable.
>
> If Robert is resistant to that but would accept it in master,
> I'd settle for that in preference to having no fix.

I'm not sure it'll help the discussion any, but on master, the
performance gap between using --load-via-partition-root and not using
it should be somewhat closed due to 3592e0ff9. So using that is likely
not as terrible as it once was.

[1] does not show results for inserting directly into partitions, but
the benchmark results do show that performance is better than it was
without the caching. The order that pg_dump outputs the rows should
mean the cache is hit most of the time for RANGE partitioned tables,
at least, and likely more often than not for LIST. HASH partitioning
is not really affected by that commit. The idea there is that it's
probably as cheap to hash as it is to do an equality check with the
last Datum.

Digging into the history a bit, I found [2] and particularly [3] that
seem to indicate this option was thought about due to concerns about
hash functions not returning consistent results on different
architectures. I suspect it might have been defaulted to load into the
leaf partitions for performance reasons, however. I mean, why else
would you?

David

[1] https://www.postgresql.org/message-id/CAApHDvqFeW5hvQqprXOLuGMMJSf%2B1C%2BWk4w_L-M03sVduF3oYg%40mail...
[2] https://www.postgresql.org/message-id/[email protected]...
[3] https://www.postgresql.org/message-id/CA%2BTgmoZFn7TJ7QBsFatnuEE%3DGYGdZSNXqr9489n5JBsdy5rFfA%40mail...






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-02 01:03  Tom Lane <[email protected]>
  parent: David Rowley <[email protected]>
  0 siblings, 1 reply; 36+ messages in thread

From: Tom Lane @ 2023-02-02 01:03 UTC (permalink / raw)
  To: David Rowley <[email protected]>; +Cc: Peter Geoghegan <[email protected]>; Robert Haas <[email protected]>; [email protected]; Andrew <[email protected]>

David Rowley <[email protected]> writes:
> Digging into the history a bit, I found [2] and particularly [3] that
> seem to indicate this option was thought about due to concerns about
> hash functions not returning consistent results on different
> architectures. I suspect it might have been defaulted to load into the
> leaf partitions for performance reasons, however. I mean, why else
> would you?
> [3] https://www.postgresql.org/message-id/CA%2BTgmoZFn7TJ7QBsFatnuEE%3DGYGdZSNXqr9489n5JBsdy5rFfA%40mail...

Hah ... so we went over almost exactly this same ground in 2017.
The consensus at that point seemed to be that actual problems
would be rare enough that we'd not need to impose the overhead
of --load-via-partition-root by default.  Now, AFAICS that was
based on exactly zero hard evidence, as to either the frequency
of actual problems or the cost of --load-via-partition-root.
Our optimism about the former seems to have been mostly borne out,
given the lack of complaints since then; but I still think our
pessimism about the latter is on shaky grounds.

Anyway, after re-reading the old thread I wonder if my first instinct
(force --load-via-partition-root for enum hash cases only) was the
best compromise after all.  I'm not sure how painful it is to get
pg_dump to detect such cases, but it's probably possible.

			regards, tom lane






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-02 09:51  Laurenz Albe <[email protected]>
  parent: Tom Lane <[email protected]>
  1 sibling, 0 replies; 36+ messages in thread

From: Laurenz Albe @ 2023-02-02 09:51 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; Robert Haas <[email protected]>; +Cc: [email protected]; Andrew <[email protected]>

On Wed, 2023-02-01 at 17:49 -0500, Tom Lane wrote:
> Robert Haas <[email protected]> writes:
> > On Wed, Feb 1, 2023 at 5:08 PM Tom Lane <[email protected]> wrote:
> > > I can agree with that argument for range or list partitioning, where
> > > the partitions have some semantic meaning to the user.  I don't buy it
> > > for hash partitioning.  It was an implementation artifact to begin
> > > with that a given row ended up in partition 3 not partition 11, so why
> > > would users care which partition it ends up in after a dump/reload?
> > > If they think there is a difference between the partitions, they need
> > > education.
> 
> > I see your point. I think it's somewhat valid. However, I also think
> > it muddies the definition of what pg_dump is allowed to do in a way
> > that I do not like. I think there's a difference between the CTID or
> > XMAX of a tuple changing and it ending up in a totally different
> > partition. It feels like it makes the definition of correctness
> > subjective: we do think that people care about range and list
> > partitions as individual entities, so we'll put the rows back where
> > they were and complain if we can't, but we don't think they think
> > about hash partitions that way, so we will err on the side of making
> > the dump restoration succeed. That's a level of guessing what the user
> > meant that I think is uncomfortable.
> 
> I see your point too, and to me it's evidence for the position that
> we should never have done hash partitioning in the first place.

You suggested earlier to deprecate hash partitioning.  That's a bit
much, but I'd say that most use cases of hash partitioning that I can
imagine would involve integers.  We could warn against using hash
partitioning for data types other than numbers and date/time in the
documentation.

I also understand the bad feeling of changing partitions during a
dump/restore, but I cannot think of a better way out.

> What do you think of "--load-via-partition-root=on/off/auto", where
> auto means "not with hash partitions" or the like?

That's perhaps the best way.  So users who know that their hash
partitions won't change and want the small speed benefit can have it.

Yours,
Laurenz Albe






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-02 10:58  Alvaro Herrera <[email protected]>
  parent: Robert Haas <[email protected]>
  1 sibling, 0 replies; 36+ messages in thread

From: Alvaro Herrera @ 2023-02-02 10:58 UTC (permalink / raw)
  To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; [email protected]; Andrew <[email protected]>

On 2023-Feb-01, Robert Haas wrote:

> I think you can construct plausible cases where it's not just
> academic. For instance, suppose I intend to use some kind of logical
> replication system, not necessarily the one built into PostgreSQL, to
> replicate data between two systems. Before engaging that system, I
> need to make the initial database contents match. The system is
> oblivious to partitioning, and just replicates each table to a table
> with a matching name.

This only works if that other system's hashing behavior is identical to
Postgres' for hashing that particular enum; there's no other way that
you could make the tables match exactly in the way you propose.  What
this tells me is that it's not really reasonable for users to expect
that this situation would actually work.  It is totally reasonable for
range and list, but not for hash.


If the idea of --load-via-partition-root=auto is going to be the fix for
this problem, then it has to consider that hash partitioning might be in
a level below the topmost one.  For example,

create type colors as enum ('blue', 'red', 'green');
create table topmost (prim int, col colors, a int) partition by range (prim);
create table parent partition of topmost for values from (0) to (1000) partition by hash (col);
create table child1 partition of parent for values with (modulus 3, remainder 0);
create table child2 partition of parent for values with (modulus 3, remainder 1);
create table child3 partition of parent for values with (modulus 3, remainder 2);

If you dump this with --load-via-partition-root, for child1 it'll give you this:

--
-- Data for Name: child1; Type: TABLE DATA; Schema: public; Owner: alvherre
--

COPY public.topmost (prim, col, a) FROM stdin;
\.

which is what we want; so for --load-via-partition-root=auto (or
whatever), we need to ensure that we detect hash partitioning all the
way down from the topmost to the leaves.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-02 12:56  Andrew Dunstan <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 1 reply; 36+ messages in thread

From: Andrew Dunstan @ 2023-02-02 12:56 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; David Rowley <[email protected]>; +Cc: Peter Geoghegan <[email protected]>; Robert Haas <[email protected]>; [email protected]; Andrew <[email protected]>


On 2023-02-01 We 20:03, Tom Lane wrote:
>
> Anyway, after re-reading the old thread I wonder if my first instinct
> (force --load-via-partition-root for enum hash cases only) was the
> best compromise after all.  I'm not sure how painful it is to get
> pg_dump to detect such cases, but it's probably possible.
>
> 			


Given the other problems you enumerated upthread, I'd be more inclined
to go with your other suggestion of
"--load-via-partition-root=on/off/auto" (with the default presumably
"auto").


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com







^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-02 14:52  Tom Lane <[email protected]>
  parent: Andrew Dunstan <[email protected]>
  0 siblings, 1 reply; 36+ messages in thread

From: Tom Lane @ 2023-02-02 14:52 UTC (permalink / raw)
  To: Andrew Dunstan <[email protected]>; +Cc: David Rowley <[email protected]>; Peter Geoghegan <[email protected]>; Robert Haas <[email protected]>; [email protected]; Andrew <[email protected]>

Andrew Dunstan <[email protected]> writes:
> On 2023-02-01 We 20:03, Tom Lane wrote:
>> Anyway, after re-reading the old thread I wonder if my first instinct
>> (force --load-via-partition-root for enum hash cases only) was the
>> best compromise after all.  I'm not sure how painful it is to get
>> pg_dump to detect such cases, but it's probably possible.

> Given the other problems you enumerated upthread, I'd be more inclined
> to go with your other suggestion of
> "--load-via-partition-root=on/off/auto" (with the default presumably
> "auto").

Hmm ... is there any actual value in "off" in this case?  We can be
just about certain that dump/reload of a hashed enum key will fail.

If we made "auto" also use --load-via-partition-root for range keys
having collation properties, there'd be more of an argument for
letting users override it.

			regards, tom lane






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-14 19:21  Tom Lane <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 2 replies; 36+ messages in thread

From: Tom Lane @ 2023-02-14 19:21 UTC (permalink / raw)
  To: Andrew Dunstan <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Rowley <[email protected]>; Peter Geoghegan <[email protected]>; Robert Haas <[email protected]>; [email protected]; Andrew <[email protected]>

Here's a set of draft patches around this issue.

0001 does what I last suggested, ie force load-via-partition-root for
leaf tables underneath a partitioned table with a partitioned-by-hash
enum column.  It wasn't quite as messy as I first feared, although we do
need a new query (and pg_dump now knows more about pg_partitioned_table
than it used to).

I was a bit unhappy to read this in the documentation:

        It is best not to use parallelism when restoring from an archive made
        with this option, because <application>pg_restore</application> will
        not know exactly which partition(s) a given archive data item will
        load data into.  This could result in inefficiency due to lock
        conflicts between parallel jobs, or perhaps even restore failures due
        to foreign key constraints being set up before all the relevant data
        is loaded.

This made me wonder if this could be a usable solution at all, but
after thinking for awhile, I don't see how the claim about foreign key
constraints is anything but FUD.  pg_dump/pg_restore have sufficient
dependency logic to prevent that from happening.  I think we can just
drop the "or perhaps ..." clause here, and tolerate the possible
inefficiency as better than failing.

0002 and 0003 are not part of the bug fix, but are some performance
improvements I noticed while working on this.  0002 is pretty minor,
but 0003 is possibly significant if you have a ton of partitions.
I haven't done any performance measurement on it though.

			regards, tom lane



Attachments:

  [text/x-diff] v1-0001-Force-load-via-partition-root-for-hash-partitioni.patch (8.7K, ../../[email protected]/2-v1-0001-Force-load-via-partition-root-for-hash-partitioni.patch)
  download | inline diff:
From 0c66c9f1211e16366cf471ef48a52e5447c79424 Mon Sep 17 00:00:00 2001
From: Tom Lane <[email protected]>
Date: Tue, 14 Feb 2023 13:09:04 -0500
Subject: [PATCH v1 1/3] Force load-via-partition-root for hash partitioning on
 an enum column.

Without this, dump and restore will almost always fail because the
enum values will receive different OIDs in the destination database,
and their hash codes will therefore be different.  (Improving the
hash algorithm would not make this situation better, and would
indeed break other cases as well.)

Discussion: https://postgr.es/m/[email protected]
---
 src/bin/pg_dump/common.c  |  18 +++---
 src/bin/pg_dump/pg_dump.c | 120 +++++++++++++++++++++++++++++++++++---
 src/bin/pg_dump/pg_dump.h |   2 +
 3 files changed, 124 insertions(+), 16 deletions(-)

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index a43f2e5553..3d0cfc1b8e 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -230,6 +230,9 @@ getSchemaData(Archive *fout, int *numTablesPtr)
 	pg_log_info("flagging inherited columns in subtables");
 	flagInhAttrs(fout->dopt, tblinfo, numTables);
 
+	pg_log_info("reading partitioning data");
+	getPartitioningInfo(fout);
+
 	pg_log_info("reading indexes");
 	getIndexes(fout, tblinfo, numTables);
 
@@ -285,7 +288,6 @@ static void
 flagInhTables(Archive *fout, TableInfo *tblinfo, int numTables,
 			  InhInfo *inhinfo, int numInherits)
 {
-	DumpOptions *dopt = fout->dopt;
 	int			i,
 				j;
 
@@ -301,18 +303,18 @@ flagInhTables(Archive *fout, TableInfo *tblinfo, int numTables,
 			continue;
 
 		/*
-		 * Normally, we don't bother computing anything for non-target tables,
-		 * but if load-via-partition-root is specified, we gather information
-		 * on every partition in the system so that getRootTableInfo can trace
-		 * from any given to leaf partition all the way up to the root.  (We
-		 * don't need to mark them as interesting for getTableAttrs, though.)
+		 * Normally, we don't bother computing anything for non-target tables.
+		 * However, we must find the parents of non-root partitioned tables in
+		 * any case, so that we can trace from leaf partitions up to the root
+		 * (in case a leaf is to be dumped but its parents are not).  We need
+		 * not mark such parents interesting for getTableAttrs, though.
 		 */
 		if (!tblinfo[i].dobj.dump)
 		{
 			mark_parents = false;
 
-			if (!dopt->load_via_partition_root ||
-				!tblinfo[i].ispartition)
+			if (!(tblinfo[i].relkind == RELKIND_PARTITIONED_TABLE &&
+				  tblinfo[i].ispartition))
 				find_parents = false;
 		}
 
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 527c7651ab..51b317aa3d 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -320,6 +320,7 @@ static void appendReloptionsArrayAH(PQExpBuffer buffer, const char *reloptions,
 static char *get_synchronized_snapshot(Archive *fout);
 static void setupDumpWorker(Archive *AH);
 static TableInfo *getRootTableInfo(const TableInfo *tbinfo);
+static bool forcePartitionRootLoad(const TableInfo *tbinfo);
 
 
 int
@@ -2228,11 +2229,13 @@ dumpTableData_insert(Archive *fout, const void *dcontext)
 			insertStmt = createPQExpBuffer();
 
 			/*
-			 * When load-via-partition-root is set, get the root table name
-			 * for the partition table, so that we can reload data through the
-			 * root table.
+			 * When load-via-partition-root is set or forced, get the root
+			 * table name for the partition table, so that we can reload data
+			 * through the root table.
 			 */
-			if (dopt->load_via_partition_root && tbinfo->ispartition)
+			if (tbinfo->ispartition &&
+				(dopt->load_via_partition_root ||
+				 forcePartitionRootLoad(tbinfo)))
 				targettab = getRootTableInfo(tbinfo);
 			else
 				targettab = tbinfo;
@@ -2430,6 +2433,35 @@ getRootTableInfo(const TableInfo *tbinfo)
 	return parentTbinfo;
 }
 
+/*
+ * forcePartitionRootLoad
+ *     Check if we must force load_via_partition_root for this partition.
+ *
+ * This is required if any level of ancestral partitioned table has an
+ * unsafe partitioning scheme.
+ */
+static bool
+forcePartitionRootLoad(const TableInfo *tbinfo)
+{
+	TableInfo  *parentTbinfo;
+
+	Assert(tbinfo->ispartition);
+	Assert(tbinfo->numParents == 1);
+
+	parentTbinfo = tbinfo->parents[0];
+	if (parentTbinfo->unsafe_partitions)
+		return true;
+	while (parentTbinfo->ispartition)
+	{
+		Assert(parentTbinfo->numParents == 1);
+		parentTbinfo = parentTbinfo->parents[0];
+		if (parentTbinfo->unsafe_partitions)
+			return true;
+	}
+
+	return false;
+}
+
 /*
  * dumpTableData -
  *	  dump the contents of a single table
@@ -2456,11 +2488,13 @@ dumpTableData(Archive *fout, const TableDataInfo *tdinfo)
 		dumpFn = dumpTableData_copy;
 
 		/*
-		 * When load-via-partition-root is set, get the root table name for
-		 * the partition table, so that we can reload data through the root
-		 * table.
+		 * When load-via-partition-root is set or forced, get the root table
+		 * name for the partition table, so that we can reload data through
+		 * the root table.
 		 */
-		if (dopt->load_via_partition_root && tbinfo->ispartition)
+		if (tbinfo->ispartition &&
+			(dopt->load_via_partition_root ||
+			 forcePartitionRootLoad(tbinfo)))
 		{
 			TableInfo  *parentTbinfo;
 
@@ -6744,6 +6778,76 @@ getInherits(Archive *fout, int *numInherits)
 	return inhinfo;
 }
 
+/*
+ * getPartitioningInfo
+ *	  get information about partitioning
+ *
+ * For the most part, we only collect partitioning info about tables we
+ * intend to dump.  However, this function has to consider all partitioned
+ * tables in the database, because we need to know about parents of partitions
+ * we are going to dump even if the parents themselves won't be dumped.
+ *
+ * Specifically, what we need to know is whether each partitioned table
+ * has an "unsafe" partitioning scheme that requires us to force
+ * load-via-partition-root mode for its children.  Currently the only case
+ * for which we force that is hash partitioning on enum columns, since the
+ * hash codes depend on enum value OIDs which won't be replicated across
+ * dump-and-reload.  There are other cases in which load-via-partition-root
+ * might be necessary, but we expect users to cope with them.
+ */
+void
+getPartitioningInfo(Archive *fout)
+{
+	PQExpBuffer query;
+	PGresult   *res;
+	int			ntups;
+
+	/* no partitions before v10 */
+	if (fout->remoteVersion < 100000)
+		return;
+	/* needn't bother if schema-only dump */
+	if (fout->dopt->schemaOnly)
+		return;
+
+	query = createPQExpBuffer();
+
+	/*
+	 * Unsafe partitioning schemes are exactly those for which hash enum_ops
+	 * appears among the partition opclasses.  We needn't check partstrat.
+	 *
+	 * Note that this query may well retrieve info about tables we aren't
+	 * going to dump and hence have no lock on.  That's okay since we need not
+	 * invoke any unsafe server-side functions.
+	 */
+	appendPQExpBufferStr(query,
+						 "SELECT partrelid FROM pg_partitioned_table WHERE\n"
+						 "(SELECT c.oid FROM pg_opclass c JOIN pg_am a "
+						 "ON c.opcmethod = a.oid\n"
+						 "WHERE opcname = 'enum_ops' "
+						 "AND opcnamespace = 'pg_catalog'::regnamespace "
+						 "AND amname = 'hash') = ANY(partclass)");
+
+	res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+
+	ntups = PQntuples(res);
+
+	for (int i = 0; i < ntups; i++)
+	{
+		Oid			tabrelid = atooid(PQgetvalue(res, i, 0));
+		TableInfo  *tbinfo;
+
+		tbinfo = findTableByOid(tabrelid);
+		if (tbinfo == NULL)
+			pg_fatal("failed sanity check, table OID %u appearing in pg_partitioned_table not found",
+					 tabrelid);
+		tbinfo->unsafe_partitions = true;
+	}
+
+	PQclear(res);
+
+	destroyPQExpBuffer(query);
+}
+
 /*
  * getIndexes
  *	  get information about every index on a dumpable table
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index e7cbd8d7ed..bfd3cf17b7 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -318,6 +318,7 @@ typedef struct _tableInfo
 	bool		dummy_view;		/* view's real definition must be postponed */
 	bool		postponed_def;	/* matview must be postponed into post-data */
 	bool		ispartition;	/* is table a partition? */
+	bool		unsafe_partitions;	/* is it an unsafe partitioned table? */
 
 	/*
 	 * These fields are computed only if we decide the table is interesting
@@ -715,6 +716,7 @@ extern ConvInfo *getConversions(Archive *fout, int *numConversions);
 extern TableInfo *getTables(Archive *fout, int *numTables);
 extern void getOwnedSeqs(Archive *fout, TableInfo tblinfo[], int numTables);
 extern InhInfo *getInherits(Archive *fout, int *numInherits);
+extern void getPartitioningInfo(Archive *fout);
 extern void getIndexes(Archive *fout, TableInfo tblinfo[], int numTables);
 extern void getExtendedStatistics(Archive *fout);
 extern void getConstraints(Archive *fout, TableInfo tblinfo[], int numTables);
-- 
2.31.1



  [text/x-diff] v1-0002-Don-t-create-useless-TableAttachInfo-objects.patch (1.5K, ../../[email protected]/3-v1-0002-Don-t-create-useless-TableAttachInfo-objects.patch)
  download | inline diff:
From 5f73193c120ce04f4e5bb3530fe1ee84afc841f1 Mon Sep 17 00:00:00 2001
From: Tom Lane <[email protected]>
Date: Tue, 14 Feb 2023 13:18:29 -0500
Subject: [PATCH v1 2/3] Don't create useless TableAttachInfo objects.

It's silly to create a TableAttachInfo object that we're not
going to dump, when we know perfectly well at creation time
that it won't be dumped.

Discussion: https://postgr.es/m/[email protected]
---
 src/bin/pg_dump/common.c  | 3 ++-
 src/bin/pg_dump/pg_dump.c | 3 ---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index 3d0cfc1b8e..ce9d2a8ee8 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -336,7 +336,8 @@ flagInhTables(Archive *fout, TableInfo *tblinfo, int numTables,
 		}
 
 		/* Create TableAttachInfo object if needed */
-		if (tblinfo[i].dobj.dump && tblinfo[i].ispartition)
+		if ((tblinfo[i].dobj.dump & DUMP_COMPONENT_DEFINITION) &&
+			tblinfo[i].ispartition)
 		{
 			TableAttachInfo *attachinfo;
 
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 51b317aa3d..35bba8765f 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -16082,9 +16082,6 @@ dumpTableAttach(Archive *fout, const TableAttachInfo *attachinfo)
 	if (dopt->dataOnly)
 		return;
 
-	if (!(attachinfo->partitionTbl->dobj.dump & DUMP_COMPONENT_DEFINITION))
-		return;
-
 	q = createPQExpBuffer();
 
 	if (!fout->is_prepared[PREPQUERY_DUMPTABLEATTACH])
-- 
2.31.1



  [text/x-diff] v1-0003-Simplify-pg_dump-s-creation-of-parent-table-links.patch (6.7K, ../../[email protected]/4-v1-0003-Simplify-pg_dump-s-creation-of-parent-table-links.patch)
  download | inline diff:
From 7cc336043e750ec154185b972970b2364f0f57ee Mon Sep 17 00:00:00 2001
From: Tom Lane <[email protected]>
Date: Tue, 14 Feb 2023 13:35:52 -0500
Subject: [PATCH v1 3/3] Simplify pg_dump's creation of parent-table links.

Instead of trying to optimize this by skipping creation of the
links for tables we don't plan to dump, just create them all in
bulk with a single scan over the pg_inherits data.  The previous
approach was more or less O(N^2) in the number of pg_inherits
entries, not to mention being way too complicated.

Discussion: https://postgr.es/m/[email protected]
---
 src/bin/pg_dump/common.c  | 125 ++++++++++++++++----------------------
 src/bin/pg_dump/pg_dump.h |   5 +-
 2 files changed, 54 insertions(+), 76 deletions(-)

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index ce9d2a8ee8..84d99ee8b6 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -83,8 +83,6 @@ static void flagInhTables(Archive *fout, TableInfo *tblinfo, int numTables,
 						  InhInfo *inhinfo, int numInherits);
 static void flagInhIndexes(Archive *fout, TableInfo *tblinfo, int numTables);
 static void flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables);
-static void findParentsByOid(TableInfo *self,
-							 InhInfo *inhinfo, int numInherits);
 static int	strInArray(const char *pattern, char **arr, int arr_size);
 static IndxInfo *findIndexByOid(Oid oid);
 
@@ -288,45 +286,70 @@ static void
 flagInhTables(Archive *fout, TableInfo *tblinfo, int numTables,
 			  InhInfo *inhinfo, int numInherits)
 {
+	TableInfo  *child = NULL;
+	TableInfo  *parent = NULL;
 	int			i,
 				j;
 
-	for (i = 0; i < numTables; i++)
+	/*
+	 * Set up links from child tables to their parents.
+	 *
+	 * We used to attempt to skip this work for tables that are not to be
+	 * dumped; but the optimizable cases are rare in practice, and setting up
+	 * these links in bulk is cheaper than the old way.  (Note in particular
+	 * that it's very rare for a child to have more than one parent.)
+	 */
+	for (i = 0; i < numInherits; i++)
 	{
-		bool		find_parents = true;
-		bool		mark_parents = true;
-
-		/* Some kinds never have parents */
-		if (tblinfo[i].relkind == RELKIND_SEQUENCE ||
-			tblinfo[i].relkind == RELKIND_VIEW ||
-			tblinfo[i].relkind == RELKIND_MATVIEW)
-			continue;
-
 		/*
-		 * Normally, we don't bother computing anything for non-target tables.
-		 * However, we must find the parents of non-root partitioned tables in
-		 * any case, so that we can trace from leaf partitions up to the root
-		 * (in case a leaf is to be dumped but its parents are not).  We need
-		 * not mark such parents interesting for getTableAttrs, though.
+		 * Skip a hashtable lookup if it's same table as last time.  This is
+		 * unlikely for the child, but less so for the parent.  (Maybe we
+		 * should ask the backend for a sorted array to make it more likely?
+		 * Not clear the sorting effort would be repaid, though.)
 		 */
-		if (!tblinfo[i].dobj.dump)
+		if (child == NULL ||
+			child->dobj.catId.oid != inhinfo[i].inhrelid)
 		{
-			mark_parents = false;
+			child = findTableByOid(inhinfo[i].inhrelid);
 
-			if (!(tblinfo[i].relkind == RELKIND_PARTITIONED_TABLE &&
-				  tblinfo[i].ispartition))
-				find_parents = false;
+			/*
+			 * If we find no TableInfo, assume the pg_inherits entry is for a
+			 * partitioned index, which we don't need to track.
+			 */
+			if (child == NULL)
+				continue;
 		}
+		if (parent == NULL ||
+			parent->dobj.catId.oid != inhinfo[i].inhparent)
+		{
+			parent = findTableByOid(inhinfo[i].inhparent);
+			if (parent == NULL)
+				pg_fatal("failed sanity check, parent OID %u of table \"%s\" (OID %u) not found",
+						 inhinfo[i].inhparent,
+						 child->dobj.name,
+						 child->dobj.catId.oid);
+		}
+		/* Add this parent to the child's list of parents. */
+		if (child->numParents > 0)
+			child->parents = pg_realloc_array(child->parents,
+											  TableInfo *,
+											  child->numParents + 1);
+		else
+			child->parents = pg_malloc_array(TableInfo *, 1);
+		child->parents[child->numParents++] = parent;
+	}
 
-		/* If needed, find all the immediate parent tables. */
-		if (find_parents)
-			findParentsByOid(&tblinfo[i], inhinfo, numInherits);
-
+	/*
+	 * Now consider all child tables and mark parents interesting as needed.
+	 */
+	for (i = 0; i < numTables; i++)
+	{
 		/*
 		 * If needed, mark the parents as interesting for getTableAttrs and
-		 * getIndexes.
+		 * getIndexes.  We only need this for direct parents of dumpable
+		 * tables.
 		 */
-		if (mark_parents)
+		if (tblinfo[i].dobj.dump)
 		{
 			int			numParents = tblinfo[i].numParents;
 			TableInfo **parents = tblinfo[i].parents;
@@ -979,52 +1002,6 @@ findOwningExtension(CatalogId catalogId)
 }
 
 
-/*
- * findParentsByOid
- *	  find a table's parents in tblinfo[]
- */
-static void
-findParentsByOid(TableInfo *self,
-				 InhInfo *inhinfo, int numInherits)
-{
-	Oid			oid = self->dobj.catId.oid;
-	int			i,
-				j;
-	int			numParents;
-
-	numParents = 0;
-	for (i = 0; i < numInherits; i++)
-	{
-		if (inhinfo[i].inhrelid == oid)
-			numParents++;
-	}
-
-	self->numParents = numParents;
-
-	if (numParents > 0)
-	{
-		self->parents = pg_malloc_array(TableInfo *, numParents);
-		j = 0;
-		for (i = 0; i < numInherits; i++)
-		{
-			if (inhinfo[i].inhrelid == oid)
-			{
-				TableInfo  *parent;
-
-				parent = findTableByOid(inhinfo[i].inhparent);
-				if (parent == NULL)
-					pg_fatal("failed sanity check, parent OID %u of table \"%s\" (OID %u) not found",
-							 inhinfo[i].inhparent,
-							 self->dobj.name,
-							 oid);
-				self->parents[j++] = parent;
-			}
-		}
-	}
-	else
-		self->parents = NULL;
-}
-
 /*
  * parseOidArray
  *	  parse a string of numbers delimited by spaces into a character array
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index bfd3cf17b7..bb4e981e7d 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -320,6 +320,9 @@ typedef struct _tableInfo
 	bool		ispartition;	/* is table a partition? */
 	bool		unsafe_partitions;	/* is it an unsafe partitioned table? */
 
+	int			numParents;		/* number of (immediate) parent tables */
+	struct _tableInfo **parents;	/* TableInfos of immediate parents */
+
 	/*
 	 * These fields are computed only if we decide the table is interesting
 	 * (it's either a table to dump, or a direct parent of a dumpable table).
@@ -352,8 +355,6 @@ typedef struct _tableInfo
 	/*
 	 * Stuff computed only for dumpable tables.
 	 */
-	int			numParents;		/* number of (immediate) parent tables */
-	struct _tableInfo **parents;	/* TableInfos of immediate parents */
 	int			numIndexes;		/* number of indexes */
 	struct _indxInfo *indexes;	/* indexes */
 	struct _tableDataInfo *dataObj; /* TableDataInfo, if dumping its data */
-- 
2.31.1



^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-02-27 15:50  Robert Haas <[email protected]>
  parent: Tom Lane <[email protected]>
  1 sibling, 0 replies; 36+ messages in thread

From: Robert Haas @ 2023-02-27 15:50 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Alvaro Herrera <[email protected]>; David Rowley <[email protected]>; Peter Geoghegan <[email protected]>; [email protected]; Andrew <[email protected]>

On Tue, Feb 14, 2023 at 2:21 PM Tom Lane <[email protected]> wrote:
> This made me wonder if this could be a usable solution at all, but
> after thinking for awhile, I don't see how the claim about foreign key
> constraints is anything but FUD.  pg_dump/pg_restore have sufficient
> dependency logic to prevent that from happening.  I think we can just
> drop the "or perhaps ..." clause here, and tolerate the possible
> inefficiency as better than failing.

Right, but isn't that dependency logic based around the fact that the
inserts are targeting the original partition? Like, suppose partition
A has a foreign key that is not present on partition B. A row that is
originally in partition B gets rerouted into partition A. It must now
satisfy the foreign key constraint when, previously, that was
unnecessary.

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






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* Re: pg_dump versus hash partitioning
@ 2023-03-11 03:01  Julien Rouhaud <[email protected]>
  parent: Tom Lane <[email protected]>
  1 sibling, 0 replies; 36+ messages in thread

From: Julien Rouhaud @ 2023-03-11 03:01 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Alvaro Herrera <[email protected]>; David Rowley <[email protected]>; Peter Geoghegan <[email protected]>; Robert Haas <[email protected]>; [email protected]; Andrew <[email protected]>

On Tue, Feb 14, 2023 at 02:21:33PM -0500, Tom Lane wrote:
> Here's a set of draft patches around this issue.
>
> 0001 does what I last suggested, ie force load-via-partition-root for
> leaf tables underneath a partitioned table with a partitioned-by-hash
> enum column.  It wasn't quite as messy as I first feared, although we do
> need a new query (and pg_dump now knows more about pg_partitioned_table
> than it used to).
>
> I was a bit unhappy to read this in the documentation:
>
>         It is best not to use parallelism when restoring from an archive made
>         with this option, because <application>pg_restore</application> will
>         not know exactly which partition(s) a given archive data item will
>         load data into.  This could result in inefficiency due to lock
>         conflicts between parallel jobs, or perhaps even restore failures due
>         to foreign key constraints being set up before all the relevant data
>         is loaded.
>
> This made me wonder if this could be a usable solution at all, but
> after thinking for awhile, I don't see how the claim about foreign key
> constraints is anything but FUD.  pg_dump/pg_restore have sufficient
> dependency logic to prevent that from happening.  I think we can just
> drop the "or perhaps ..." clause here, and tolerate the possible
> inefficiency as better than failing.

Working on some side project that can cause dump of hash partitions to be
routed to a different partition, I realized that --load-via-partition-root can
indeed cause deadlock in such case without FK dependency or anything else.

The problem is that each worker will perform a TRUNCATE TABLE ONLY followed by
a copy of the original partition's data in a transaction, and that obviously
will lead to deadlock if the original and locked partition and the restored
partition are different.






^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* [PATCH v2 06/17] lazy_scan_prune reorder freeze execution logic
@ 2024-01-07 19:50  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 36+ messages in thread

From: Melanie Plageman @ 2024-01-07 19:50 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning. It also adds a helper calculating freeze snapshot conflict
horizon. This will be useful when the freeze execution is moved into
pruning because not all callers of heap_page_prune() have access to
VacuumCutoffs.
---
 src/backend/access/heap/vacuumlazy.c | 112 ++++++++++++++++-----------
 1 file changed, 67 insertions(+), 45 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..abbb7ab3ada 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -269,6 +269,8 @@ static void update_vacuum_error_info(LVRelState *vacrel,
 static void restore_vacuum_error_info(LVRelState *vacrel,
 									  const LVSavedErrInfo *saved_vacrel);
 
+static TransactionId heap_frz_conflict_horizon(PruneResult *presult,
+											   HeapPageFreeze *pagefrz);
 
 /*
  *	heap_vacuum_rel() -- perform VACUUM for one heap relation
@@ -1373,6 +1375,33 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
 	return false;
 }
 
+/*
+ * Determine the snapshotConflictHorizon for freezing. Must only be called
+ * after pruning and determining if the page is freezable.
+ */
+static TransactionId
+heap_frz_conflict_horizon(PruneResult *presult, HeapPageFreeze *pagefrz)
+{
+	TransactionId result;
+
+	/*
+	 * We can use frz_conflict_horizon as our cutoff for conflicts when the
+	 * whole page is eligible to become all-frozen in the VM once we're done
+	 * with it.  Otherwise we generate a conservative cutoff by stepping back
+	 * from OldestXmin.
+	 */
+	if (presult->all_visible_except_removable && presult->all_frozen)
+		result = presult->frz_conflict_horizon;
+	else
+	{
+		/* Avoids false conflicts when hot_standby_feedback in use */
+		result = pagefrz->cutoffs->OldestXmin;
+		TransactionIdRetreat(result);
+	}
+
+	return result;
+}
+
 /*
  *	lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
  *
@@ -1421,6 +1450,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1610,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1626,39 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
-		else
-		{
-			TransactionId snapshotConflictHorizon;
+		vacrel->frozen_pages++;
 
-			vacrel->frozen_pages++;
+		snapshotConflictHorizon = heap_frz_conflict_horizon(&presult, &pagefrz);
 
-			/*
-			 * We can use frz_conflict_horizon as our cutoff for conflicts
-			 * when the whole page is eligible to become all-frozen in the VM
-			 * once we're done with it.  Otherwise we generate a conservative
-			 * cutoff by stepping back from OldestXmin.
-			 */
-			if (presult.all_visible_except_removable && presult.all_frozen)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--rdqtp5puvxqotfdw
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0007-Execute-freezing-in-heap_page_prune.patch"



^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* [PATCH v2 06/17] lazy_scan_prune reorder freeze execution logic
@ 2024-01-07 19:50  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 36+ messages in thread

From: Melanie Plageman @ 2024-01-07 19:50 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning. It also adds a helper calculating freeze snapshot conflict
horizon. This will be useful when the freeze execution is moved into
pruning because not all callers of heap_page_prune() have access to
VacuumCutoffs.
---
 src/backend/access/heap/vacuumlazy.c | 112 ++++++++++++++++-----------
 1 file changed, 67 insertions(+), 45 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..abbb7ab3ada 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -269,6 +269,8 @@ static void update_vacuum_error_info(LVRelState *vacrel,
 static void restore_vacuum_error_info(LVRelState *vacrel,
 									  const LVSavedErrInfo *saved_vacrel);
 
+static TransactionId heap_frz_conflict_horizon(PruneResult *presult,
+											   HeapPageFreeze *pagefrz);
 
 /*
  *	heap_vacuum_rel() -- perform VACUUM for one heap relation
@@ -1373,6 +1375,33 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
 	return false;
 }
 
+/*
+ * Determine the snapshotConflictHorizon for freezing. Must only be called
+ * after pruning and determining if the page is freezable.
+ */
+static TransactionId
+heap_frz_conflict_horizon(PruneResult *presult, HeapPageFreeze *pagefrz)
+{
+	TransactionId result;
+
+	/*
+	 * We can use frz_conflict_horizon as our cutoff for conflicts when the
+	 * whole page is eligible to become all-frozen in the VM once we're done
+	 * with it.  Otherwise we generate a conservative cutoff by stepping back
+	 * from OldestXmin.
+	 */
+	if (presult->all_visible_except_removable && presult->all_frozen)
+		result = presult->frz_conflict_horizon;
+	else
+	{
+		/* Avoids false conflicts when hot_standby_feedback in use */
+		result = pagefrz->cutoffs->OldestXmin;
+		TransactionIdRetreat(result);
+	}
+
+	return result;
+}
+
 /*
  *	lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
  *
@@ -1421,6 +1450,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1610,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1626,39 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
-		else
-		{
-			TransactionId snapshotConflictHorizon;
+		vacrel->frozen_pages++;
 
-			vacrel->frozen_pages++;
+		snapshotConflictHorizon = heap_frz_conflict_horizon(&presult, &pagefrz);
 
-			/*
-			 * We can use frz_conflict_horizon as our cutoff for conflicts
-			 * when the whole page is eligible to become all-frozen in the VM
-			 * once we're done with it.  Otherwise we generate a conservative
-			 * cutoff by stepping back from OldestXmin.
-			 */
-			if (presult.all_visible_except_removable && presult.all_frozen)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--rdqtp5puvxqotfdw
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0007-Execute-freezing-in-heap_page_prune.patch"



^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* [PATCH v3 06/17] lazy_scan_prune reorder freeze execution logic
@ 2024-01-07 19:50  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 36+ messages in thread

From: Melanie Plageman @ 2024-01-07 19:50 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning. It also adds a helper calculating freeze snapshot conflict
horizon. This will be useful when the freeze execution is moved into
pruning because not all callers of heap_page_prune() have access to
VacuumCutoffs.
---
 src/backend/access/heap/vacuumlazy.c | 112 ++++++++++++++++-----------
 1 file changed, 67 insertions(+), 45 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..abbb7ab3ada 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -269,6 +269,8 @@ static void update_vacuum_error_info(LVRelState *vacrel,
 static void restore_vacuum_error_info(LVRelState *vacrel,
 									  const LVSavedErrInfo *saved_vacrel);
 
+static TransactionId heap_frz_conflict_horizon(PruneResult *presult,
+											   HeapPageFreeze *pagefrz);
 
 /*
  *	heap_vacuum_rel() -- perform VACUUM for one heap relation
@@ -1373,6 +1375,33 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
 	return false;
 }
 
+/*
+ * Determine the snapshotConflictHorizon for freezing. Must only be called
+ * after pruning and determining if the page is freezable.
+ */
+static TransactionId
+heap_frz_conflict_horizon(PruneResult *presult, HeapPageFreeze *pagefrz)
+{
+	TransactionId result;
+
+	/*
+	 * We can use frz_conflict_horizon as our cutoff for conflicts when the
+	 * whole page is eligible to become all-frozen in the VM once we're done
+	 * with it.  Otherwise we generate a conservative cutoff by stepping back
+	 * from OldestXmin.
+	 */
+	if (presult->all_visible_except_removable && presult->all_frozen)
+		result = presult->frz_conflict_horizon;
+	else
+	{
+		/* Avoids false conflicts when hot_standby_feedback in use */
+		result = pagefrz->cutoffs->OldestXmin;
+		TransactionIdRetreat(result);
+	}
+
+	return result;
+}
+
 /*
  *	lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
  *
@@ -1421,6 +1450,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1610,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1626,39 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
-		else
-		{
-			TransactionId snapshotConflictHorizon;
+		vacrel->frozen_pages++;
 
-			vacrel->frozen_pages++;
+		snapshotConflictHorizon = heap_frz_conflict_horizon(&presult, &pagefrz);
 
-			/*
-			 * We can use frz_conflict_horizon as our cutoff for conflicts
-			 * when the whole page is eligible to become all-frozen in the VM
-			 * once we're done with it.  Otherwise we generate a conservative
-			 * cutoff by stepping back from OldestXmin.
-			 */
-			if (presult.all_visible_except_removable && presult.all_frozen)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--racicctn4wry6xe5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0007-Execute-freezing-in-heap_page_prune.patch"



^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* [PATCH v3 06/17] lazy_scan_prune reorder freeze execution logic
@ 2024-01-07 19:50  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 36+ messages in thread

From: Melanie Plageman @ 2024-01-07 19:50 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning. It also adds a helper calculating freeze snapshot conflict
horizon. This will be useful when the freeze execution is moved into
pruning because not all callers of heap_page_prune() have access to
VacuumCutoffs.
---
 src/backend/access/heap/vacuumlazy.c | 112 ++++++++++++++++-----------
 1 file changed, 67 insertions(+), 45 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..abbb7ab3ada 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -269,6 +269,8 @@ static void update_vacuum_error_info(LVRelState *vacrel,
 static void restore_vacuum_error_info(LVRelState *vacrel,
 									  const LVSavedErrInfo *saved_vacrel);
 
+static TransactionId heap_frz_conflict_horizon(PruneResult *presult,
+											   HeapPageFreeze *pagefrz);
 
 /*
  *	heap_vacuum_rel() -- perform VACUUM for one heap relation
@@ -1373,6 +1375,33 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
 	return false;
 }
 
+/*
+ * Determine the snapshotConflictHorizon for freezing. Must only be called
+ * after pruning and determining if the page is freezable.
+ */
+static TransactionId
+heap_frz_conflict_horizon(PruneResult *presult, HeapPageFreeze *pagefrz)
+{
+	TransactionId result;
+
+	/*
+	 * We can use frz_conflict_horizon as our cutoff for conflicts when the
+	 * whole page is eligible to become all-frozen in the VM once we're done
+	 * with it.  Otherwise we generate a conservative cutoff by stepping back
+	 * from OldestXmin.
+	 */
+	if (presult->all_visible_except_removable && presult->all_frozen)
+		result = presult->frz_conflict_horizon;
+	else
+	{
+		/* Avoids false conflicts when hot_standby_feedback in use */
+		result = pagefrz->cutoffs->OldestXmin;
+		TransactionIdRetreat(result);
+	}
+
+	return result;
+}
+
 /*
  *	lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
  *
@@ -1421,6 +1450,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1610,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1626,39 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
-		else
-		{
-			TransactionId snapshotConflictHorizon;
+		vacrel->frozen_pages++;
 
-			vacrel->frozen_pages++;
+		snapshotConflictHorizon = heap_frz_conflict_horizon(&presult, &pagefrz);
 
-			/*
-			 * We can use frz_conflict_horizon as our cutoff for conflicts
-			 * when the whole page is eligible to become all-frozen in the VM
-			 * once we're done with it.  Otherwise we generate a conservative
-			 * cutoff by stepping back from OldestXmin.
-			 */
-			if (presult.all_visible_except_removable && presult.all_frozen)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--racicctn4wry6xe5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0007-Execute-freezing-in-heap_page_prune.patch"



^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* [PATCH v2 06/17] lazy_scan_prune reorder freeze execution logic
@ 2024-01-07 19:50  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 36+ messages in thread

From: Melanie Plageman @ 2024-01-07 19:50 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning. It also adds a helper calculating freeze snapshot conflict
horizon. This will be useful when the freeze execution is moved into
pruning because not all callers of heap_page_prune() have access to
VacuumCutoffs.
---
 src/backend/access/heap/vacuumlazy.c | 112 ++++++++++++++++-----------
 1 file changed, 67 insertions(+), 45 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..abbb7ab3ada 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -269,6 +269,8 @@ static void update_vacuum_error_info(LVRelState *vacrel,
 static void restore_vacuum_error_info(LVRelState *vacrel,
 									  const LVSavedErrInfo *saved_vacrel);
 
+static TransactionId heap_frz_conflict_horizon(PruneResult *presult,
+											   HeapPageFreeze *pagefrz);
 
 /*
  *	heap_vacuum_rel() -- perform VACUUM for one heap relation
@@ -1373,6 +1375,33 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
 	return false;
 }
 
+/*
+ * Determine the snapshotConflictHorizon for freezing. Must only be called
+ * after pruning and determining if the page is freezable.
+ */
+static TransactionId
+heap_frz_conflict_horizon(PruneResult *presult, HeapPageFreeze *pagefrz)
+{
+	TransactionId result;
+
+	/*
+	 * We can use frz_conflict_horizon as our cutoff for conflicts when the
+	 * whole page is eligible to become all-frozen in the VM once we're done
+	 * with it.  Otherwise we generate a conservative cutoff by stepping back
+	 * from OldestXmin.
+	 */
+	if (presult->all_visible_except_removable && presult->all_frozen)
+		result = presult->frz_conflict_horizon;
+	else
+	{
+		/* Avoids false conflicts when hot_standby_feedback in use */
+		result = pagefrz->cutoffs->OldestXmin;
+		TransactionIdRetreat(result);
+	}
+
+	return result;
+}
+
 /*
  *	lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
  *
@@ -1421,6 +1450,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1610,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1626,39 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
-		else
-		{
-			TransactionId snapshotConflictHorizon;
+		vacrel->frozen_pages++;
 
-			vacrel->frozen_pages++;
+		snapshotConflictHorizon = heap_frz_conflict_horizon(&presult, &pagefrz);
 
-			/*
-			 * We can use frz_conflict_horizon as our cutoff for conflicts
-			 * when the whole page is eligible to become all-frozen in the VM
-			 * once we're done with it.  Otherwise we generate a conservative
-			 * cutoff by stepping back from OldestXmin.
-			 */
-			if (presult.all_visible_except_removable && presult.all_frozen)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--rdqtp5puvxqotfdw
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0007-Execute-freezing-in-heap_page_prune.patch"



^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* [PATCH v3 06/17] lazy_scan_prune reorder freeze execution logic
@ 2024-01-07 19:50  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 36+ messages in thread

From: Melanie Plageman @ 2024-01-07 19:50 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning. It also adds a helper calculating freeze snapshot conflict
horizon. This will be useful when the freeze execution is moved into
pruning because not all callers of heap_page_prune() have access to
VacuumCutoffs.
---
 src/backend/access/heap/vacuumlazy.c | 112 ++++++++++++++++-----------
 1 file changed, 67 insertions(+), 45 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..abbb7ab3ada 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -269,6 +269,8 @@ static void update_vacuum_error_info(LVRelState *vacrel,
 static void restore_vacuum_error_info(LVRelState *vacrel,
 									  const LVSavedErrInfo *saved_vacrel);
 
+static TransactionId heap_frz_conflict_horizon(PruneResult *presult,
+											   HeapPageFreeze *pagefrz);
 
 /*
  *	heap_vacuum_rel() -- perform VACUUM for one heap relation
@@ -1373,6 +1375,33 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
 	return false;
 }
 
+/*
+ * Determine the snapshotConflictHorizon for freezing. Must only be called
+ * after pruning and determining if the page is freezable.
+ */
+static TransactionId
+heap_frz_conflict_horizon(PruneResult *presult, HeapPageFreeze *pagefrz)
+{
+	TransactionId result;
+
+	/*
+	 * We can use frz_conflict_horizon as our cutoff for conflicts when the
+	 * whole page is eligible to become all-frozen in the VM once we're done
+	 * with it.  Otherwise we generate a conservative cutoff by stepping back
+	 * from OldestXmin.
+	 */
+	if (presult->all_visible_except_removable && presult->all_frozen)
+		result = presult->frz_conflict_horizon;
+	else
+	{
+		/* Avoids false conflicts when hot_standby_feedback in use */
+		result = pagefrz->cutoffs->OldestXmin;
+		TransactionIdRetreat(result);
+	}
+
+	return result;
+}
+
 /*
  *	lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
  *
@@ -1421,6 +1450,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1610,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1626,39 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
-		else
-		{
-			TransactionId snapshotConflictHorizon;
+		vacrel->frozen_pages++;
 
-			vacrel->frozen_pages++;
+		snapshotConflictHorizon = heap_frz_conflict_horizon(&presult, &pagefrz);
 
-			/*
-			 * We can use frz_conflict_horizon as our cutoff for conflicts
-			 * when the whole page is eligible to become all-frozen in the VM
-			 * once we're done with it.  Otherwise we generate a conservative
-			 * cutoff by stepping back from OldestXmin.
-			 */
-			if (presult.all_visible_except_removable && presult.all_frozen)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--racicctn4wry6xe5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0007-Execute-freezing-in-heap_page_prune.patch"



^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* [PATCH v4 08/19] lazy_scan_prune reorder freeze execution logic
@ 2024-03-19 23:30  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 36+ messages in thread

From: Melanie Plageman @ 2024-03-19 23:30 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning.
---
 src/backend/access/heap/vacuumlazy.c | 92 +++++++++++++++-------------
 1 file changed, 49 insertions(+), 43 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..74ebab25a95 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1421,6 +1421,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1581,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1597,52 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
+		vacrel->frozen_pages++;
+
+		/*
+		 * We can use frz_conflict_horizon as our cutoff for conflicts when
+		 * the whole page is eligible to become all-frozen in the VM once
+		 * we're done with it.  Otherwise we generate a conservative cutoff by
+		 * stepping back from OldestXmin.
+		 */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			snapshotConflictHorizon = presult.frz_conflict_horizon;
 		else
 		{
-			TransactionId snapshotConflictHorizon;
+			/* Avoids false conflicts when hot_standby_feedback in use */
+			snapshotConflictHorizon = pagefrz.cutoffs->OldestXmin;
+			TransactionIdRetreat(snapshotConflictHorizon);
+		}
 
-			vacrel->frozen_pages++;
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/*
-			 * We can use frz_conflict_horizon as our cutoff for conflicts
-			 * when the whole page is eligible to become all-frozen in the VM
-			 * once we're done with it.  Otherwise we generate a conservative
-			 * cutoff by stepping back from OldestXmin.
-			 */
-			if (presult.all_visible_except_removable && presult.all_frozen)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--tez7m2a73jtztiij
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0009-Execute-freezing-in-heap_page_prune.patch"



^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* [PATCH v4 08/19] lazy_scan_prune reorder freeze execution logic
@ 2024-03-19 23:30  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 36+ messages in thread

From: Melanie Plageman @ 2024-03-19 23:30 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning.
---
 src/backend/access/heap/vacuumlazy.c | 92 +++++++++++++++-------------
 1 file changed, 49 insertions(+), 43 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..74ebab25a95 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1421,6 +1421,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1581,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1597,52 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
+		vacrel->frozen_pages++;
+
+		/*
+		 * We can use frz_conflict_horizon as our cutoff for conflicts when
+		 * the whole page is eligible to become all-frozen in the VM once
+		 * we're done with it.  Otherwise we generate a conservative cutoff by
+		 * stepping back from OldestXmin.
+		 */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			snapshotConflictHorizon = presult.frz_conflict_horizon;
 		else
 		{
-			TransactionId snapshotConflictHorizon;
+			/* Avoids false conflicts when hot_standby_feedback in use */
+			snapshotConflictHorizon = pagefrz.cutoffs->OldestXmin;
+			TransactionIdRetreat(snapshotConflictHorizon);
+		}
 
-			vacrel->frozen_pages++;
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/*
-			 * We can use frz_conflict_horizon as our cutoff for conflicts
-			 * when the whole page is eligible to become all-frozen in the VM
-			 * once we're done with it.  Otherwise we generate a conservative
-			 * cutoff by stepping back from OldestXmin.
-			 */
-			if (presult.all_visible_except_removable && presult.all_frozen)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--tez7m2a73jtztiij
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0009-Execute-freezing-in-heap_page_prune.patch"



^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* [PATCH v4 08/19] lazy_scan_prune reorder freeze execution logic
@ 2024-03-19 23:30  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 36+ messages in thread

From: Melanie Plageman @ 2024-03-19 23:30 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning.
---
 src/backend/access/heap/vacuumlazy.c | 92 +++++++++++++++-------------
 1 file changed, 49 insertions(+), 43 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..74ebab25a95 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1421,6 +1421,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1580,10 +1581,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1597,52 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
+		vacrel->frozen_pages++;
+
+		/*
+		 * We can use frz_conflict_horizon as our cutoff for conflicts when
+		 * the whole page is eligible to become all-frozen in the VM once
+		 * we're done with it.  Otherwise we generate a conservative cutoff by
+		 * stepping back from OldestXmin.
+		 */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			snapshotConflictHorizon = presult.frz_conflict_horizon;
 		else
 		{
-			TransactionId snapshotConflictHorizon;
+			/* Avoids false conflicts when hot_standby_feedback in use */
+			snapshotConflictHorizon = pagefrz.cutoffs->OldestXmin;
+			TransactionIdRetreat(snapshotConflictHorizon);
+		}
 
-			vacrel->frozen_pages++;
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.frz_conflict_horizon = InvalidTransactionId;
 
-			/*
-			 * We can use frz_conflict_horizon as our cutoff for conflicts
-			 * when the whole page is eligible to become all-frozen in the VM
-			 * once we're done with it.  Otherwise we generate a conservative
-			 * cutoff by stepping back from OldestXmin.
-			 */
-			if (presult.all_visible_except_removable && presult.all_frozen)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.frz_conflict_horizon;
-				presult.frz_conflict_horizon = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--tez7m2a73jtztiij
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0009-Execute-freezing-in-heap_page_prune.patch"



^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* [PATCH v9 07/21] lazy_scan_prune reorder freeze execution logic
@ 2024-03-25 23:39  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 36+ messages in thread

From: Melanie Plageman @ 2024-03-25 23:39 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning.
---
 src/backend/access/heap/vacuumlazy.c | 93 +++++++++++++++-------------
 1 file changed, 50 insertions(+), 43 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 2a3cc5c7cd3..f474e661428 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1421,6 +1421,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1576,10 +1577,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1587,52 +1593,53 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
+		vacrel->frozen_pages++;
+
+		/*
+		 * We can use frz_conflict_horizon as our cutoff for conflicts when
+		 * the whole page is eligible to become all-frozen in the VM once
+		 * we're done with it.  Otherwise we generate a conservative cutoff by
+		 * stepping back from OldestXmin.
+		 */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			snapshotConflictHorizon = presult.visibility_cutoff_xid;
 		else
 		{
-			TransactionId snapshotConflictHorizon;
+			/* Avoids false conflicts when hot_standby_feedback in use */
+			snapshotConflictHorizon = pagefrz.cutoffs->OldestXmin;
+			TransactionIdRetreat(snapshotConflictHorizon);
+		}
 
-			vacrel->frozen_pages++;
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.visibility_cutoff_xid = InvalidTransactionId;
 
-			/*
-			 * We can use visibility_cutoff_xid as our cutoff for conflicts
-			 * when the whole page is eligible to become all-frozen in the VM
-			 * once we're done with it.  Otherwise we generate a conservative
-			 * cutoff by stepping back from OldestXmin.
-			 */
-			if (presult.all_visible_except_removable && presult.all_frozen)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.visibility_cutoff_xid;
-				presult.visibility_cutoff_xid = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--caj67xgx3lukmr5f
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v9-0008-Execute-freezing-in-heap_page_prune.patch"



^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* [PATCH v9 07/21] lazy_scan_prune reorder freeze execution logic
@ 2024-03-25 23:39  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 36+ messages in thread

From: Melanie Plageman @ 2024-03-25 23:39 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning.
---
 src/backend/access/heap/vacuumlazy.c | 93 +++++++++++++++-------------
 1 file changed, 50 insertions(+), 43 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 2a3cc5c7cd3..f474e661428 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1421,6 +1421,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1576,10 +1577,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1587,52 +1593,53 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
+		vacrel->frozen_pages++;
+
+		/*
+		 * We can use frz_conflict_horizon as our cutoff for conflicts when
+		 * the whole page is eligible to become all-frozen in the VM once
+		 * we're done with it.  Otherwise we generate a conservative cutoff by
+		 * stepping back from OldestXmin.
+		 */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			snapshotConflictHorizon = presult.visibility_cutoff_xid;
 		else
 		{
-			TransactionId snapshotConflictHorizon;
+			/* Avoids false conflicts when hot_standby_feedback in use */
+			snapshotConflictHorizon = pagefrz.cutoffs->OldestXmin;
+			TransactionIdRetreat(snapshotConflictHorizon);
+		}
 
-			vacrel->frozen_pages++;
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.visibility_cutoff_xid = InvalidTransactionId;
 
-			/*
-			 * We can use visibility_cutoff_xid as our cutoff for conflicts
-			 * when the whole page is eligible to become all-frozen in the VM
-			 * once we're done with it.  Otherwise we generate a conservative
-			 * cutoff by stepping back from OldestXmin.
-			 */
-			if (presult.all_visible_except_removable && presult.all_frozen)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.visibility_cutoff_xid;
-				presult.visibility_cutoff_xid = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--caj67xgx3lukmr5f
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v9-0008-Execute-freezing-in-heap_page_prune.patch"



^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* [PATCH v7 07/16] lazy_scan_prune reorder freeze execution logic
@ 2024-03-25 23:39  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 36+ messages in thread

From: Melanie Plageman @ 2024-03-25 23:39 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning.
---
 src/backend/access/heap/vacuumlazy.c | 93 +++++++++++++++-------------
 1 file changed, 50 insertions(+), 43 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 2a3cc5c7cd3..f474e661428 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1421,6 +1421,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1576,10 +1577,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1587,52 +1593,53 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
+		vacrel->frozen_pages++;
+
+		/*
+		 * We can use frz_conflict_horizon as our cutoff for conflicts when
+		 * the whole page is eligible to become all-frozen in the VM once
+		 * we're done with it.  Otherwise we generate a conservative cutoff by
+		 * stepping back from OldestXmin.
+		 */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			snapshotConflictHorizon = presult.visibility_cutoff_xid;
 		else
 		{
-			TransactionId snapshotConflictHorizon;
+			/* Avoids false conflicts when hot_standby_feedback in use */
+			snapshotConflictHorizon = pagefrz.cutoffs->OldestXmin;
+			TransactionIdRetreat(snapshotConflictHorizon);
+		}
 
-			vacrel->frozen_pages++;
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.visibility_cutoff_xid = InvalidTransactionId;
 
-			/*
-			 * We can use visibility_cutoff_xid as our cutoff for conflicts
-			 * when the whole page is eligible to become all-frozen in the VM
-			 * once we're done with it.  Otherwise we generate a conservative
-			 * cutoff by stepping back from OldestXmin.
-			 */
-			if (presult.all_visible_except_removable && presult.all_frozen)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.visibility_cutoff_xid;
-				presult.visibility_cutoff_xid = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--ck6erxojvlx23byk
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v7-0008-Execute-freezing-in-heap_page_prune.patch"



^ permalink  raw  reply  [nested|flat] 36+ messages in thread

* [PATCH v7 07/16] lazy_scan_prune reorder freeze execution logic
@ 2024-03-25 23:39  Melanie Plageman <[email protected]>
  0 siblings, 0 replies; 36+ messages in thread

From: Melanie Plageman @ 2024-03-25 23:39 UTC (permalink / raw)

To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.

This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning.
---
 src/backend/access/heap/vacuumlazy.c | 93 +++++++++++++++-------------
 1 file changed, 50 insertions(+), 43 deletions(-)

diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 2a3cc5c7cd3..f474e661428 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1421,6 +1421,7 @@ lazy_scan_prune(LVRelState *vacrel,
 				recently_dead_tuples;
 	HeapPageFreeze pagefrz;
 	bool		hastup = false;
+	bool		do_freeze;
 	int64		fpi_before = pgWalUsage.wal_fpi;
 	OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
 
@@ -1576,10 +1577,15 @@ lazy_scan_prune(LVRelState *vacrel,
 	 * freeze when pruning generated an FPI, if doing so means that we set the
 	 * page all-frozen afterwards (might not happen until final heap pass).
 	 */
-	if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+	do_freeze = pagefrz.freeze_required ||
 		(presult.all_visible_except_removable && presult.all_frozen &&
-		 fpi_before != pgWalUsage.wal_fpi))
+		 presult.nfrozen > 0 &&
+		 fpi_before != pgWalUsage.wal_fpi);
+
+	if (do_freeze)
 	{
+		TransactionId snapshotConflictHorizon;
+
 		/*
 		 * We're freezing the page.  Our final NewRelfrozenXid doesn't need to
 		 * be affected by the XIDs that are just about to be frozen anyway.
@@ -1587,52 +1593,53 @@ lazy_scan_prune(LVRelState *vacrel,
 		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
 		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 
-		if (presult.nfrozen == 0)
-		{
-			/*
-			 * We have no freeze plans to execute, so there's no added cost
-			 * from following the freeze path.  That's why it was chosen. This
-			 * is important in the case where the page only contains totally
-			 * frozen tuples at this point (perhaps only following pruning).
-			 * Such pages can be marked all-frozen in the VM by our caller,
-			 * even though none of its tuples were newly frozen here (note
-			 * that the "no freeze" path never sets pages all-frozen).
-			 *
-			 * We never increment the frozen_pages instrumentation counter
-			 * here, since it only counts pages with newly frozen tuples
-			 * (don't confuse that with pages newly set all-frozen in VM).
-			 */
-		}
+		vacrel->frozen_pages++;
+
+		/*
+		 * We can use frz_conflict_horizon as our cutoff for conflicts when
+		 * the whole page is eligible to become all-frozen in the VM once
+		 * we're done with it.  Otherwise we generate a conservative cutoff by
+		 * stepping back from OldestXmin.
+		 */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			snapshotConflictHorizon = presult.visibility_cutoff_xid;
 		else
 		{
-			TransactionId snapshotConflictHorizon;
+			/* Avoids false conflicts when hot_standby_feedback in use */
+			snapshotConflictHorizon = pagefrz.cutoffs->OldestXmin;
+			TransactionIdRetreat(snapshotConflictHorizon);
+		}
 
-			vacrel->frozen_pages++;
+		/* Using same cutoff when setting VM is now unnecessary */
+		if (presult.all_visible_except_removable && presult.all_frozen)
+			presult.visibility_cutoff_xid = InvalidTransactionId;
 
-			/*
-			 * We can use visibility_cutoff_xid as our cutoff for conflicts
-			 * when the whole page is eligible to become all-frozen in the VM
-			 * once we're done with it.  Otherwise we generate a conservative
-			 * cutoff by stepping back from OldestXmin.
-			 */
-			if (presult.all_visible_except_removable && presult.all_frozen)
-			{
-				/* Using same cutoff when setting VM is now unnecessary */
-				snapshotConflictHorizon = presult.visibility_cutoff_xid;
-				presult.visibility_cutoff_xid = InvalidTransactionId;
-			}
-			else
-			{
-				/* Avoids false conflicts when hot_standby_feedback in use */
-				snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
-				TransactionIdRetreat(snapshotConflictHorizon);
-			}
+		/* Execute all freeze plans for page as a single atomic action */
+		heap_freeze_execute_prepared(vacrel->rel, buf,
+									 snapshotConflictHorizon,
+									 presult.frozen, presult.nfrozen);
 
-			/* Execute all freeze plans for page as a single atomic action */
-			heap_freeze_execute_prepared(vacrel->rel, buf,
-										 snapshotConflictHorizon,
-										 presult.frozen, presult.nfrozen);
-		}
+	}
+	else if (presult.all_frozen && presult.nfrozen == 0)
+	{
+		/* Page should be all visible except to-be-removed tuples */
+		Assert(presult.all_visible_except_removable);
+
+		/*
+		 * We have no freeze plans to execute, so there's no added cost from
+		 * following the freeze path.  That's why it was chosen. This is
+		 * important in the case where the page only contains totally frozen
+		 * tuples at this point (perhaps only following pruning). Such pages
+		 * can be marked all-frozen in the VM by our caller, even though none
+		 * of its tuples were newly frozen here (note that the "no freeze"
+		 * path never sets pages all-frozen).
+		 *
+		 * We never increment the frozen_pages instrumentation counter here,
+		 * since it only counts pages with newly frozen tuples (don't confuse
+		 * that with pages newly set all-frozen in VM).
+		 */
+		vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+		vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
 	}
 	else
 	{
-- 
2.40.1


--ck6erxojvlx23byk
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v7-0008-Execute-freezing-in-heap_page_prune.patch"



^ permalink  raw  reply  [nested|flat] 36+ messages in thread

end of thread, other threads:[~2024-03-25 23:39 UTC | newest]

Thread overview: 36+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-02-01 16:17 pg_dump versus hash partitioning Tom Lane <[email protected]>
2023-02-01 17:39 ` Robert Haas <[email protected]>
2023-02-01 20:34   ` Tom Lane <[email protected]>
2023-02-01 21:12     ` Peter Geoghegan <[email protected]>
2023-02-01 21:14     ` Robert Haas <[email protected]>
2023-02-01 21:44       ` Peter Geoghegan <[email protected]>
2023-02-01 22:12         ` Robert Haas <[email protected]>
2023-02-01 22:33           ` Tom Lane <[email protected]>
2023-02-01 22:33           ` Peter Geoghegan <[email protected]>
2023-02-01 22:38             ` Tom Lane <[email protected]>
2023-02-02 00:15               ` David Rowley <[email protected]>
2023-02-02 01:03                 ` Tom Lane <[email protected]>
2023-02-02 12:56                   ` Andrew Dunstan <[email protected]>
2023-02-02 14:52                     ` Tom Lane <[email protected]>
2023-02-14 19:21                       ` Tom Lane <[email protected]>
2023-02-27 15:50                         ` Robert Haas <[email protected]>
2023-03-11 03:01                         ` Julien Rouhaud <[email protected]>
2023-02-01 22:08       ` Tom Lane <[email protected]>
2023-02-01 22:36         ` Robert Haas <[email protected]>
2023-02-01 22:49           ` Tom Lane <[email protected]>
2023-02-01 23:15             ` Peter Geoghegan <[email protected]>
2023-02-02 09:51             ` Laurenz Albe <[email protected]>
2023-02-02 10:58           ` Alvaro Herrera <[email protected]>
2024-01-07 19:50 [PATCH v3 06/17] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-01-07 19:50 [PATCH v3 06/17] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-01-07 19:50 [PATCH v2 06/17] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-01-07 19:50 [PATCH v2 06/17] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-01-07 19:50 [PATCH v2 06/17] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-01-07 19:50 [PATCH v3 06/17] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-03-19 23:30 [PATCH v4 08/19] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-03-19 23:30 [PATCH v4 08/19] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-03-19 23:30 [PATCH v4 08/19] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-03-25 23:39 [PATCH v9 07/21] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-03-25 23:39 [PATCH v7 07/16] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-03-25 23:39 [PATCH v7 07/16] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
2024-03-25 23:39 [PATCH v9 07/21] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox