public inbox for [email protected]
help / color / mirror / Atom feedRe: pl/pgsql feature request: shorthand for argument and local variable references
30+ messages / 6 participants
[nested] [flat]
* Re: pl/pgsql feature request: shorthand for argument and local variable references
@ 2022-01-06 07:02 Julien Rouhaud <[email protected]>
2022-01-06 07:52 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Julien Rouhaud @ 2022-01-06 07:02 UTC (permalink / raw)
To: Pavel Stehule <[email protected]>; +Cc: Michael Paquier <[email protected]>; Hannu Krosing <[email protected]>; Vik Fearing <[email protected]>; Chapman Flack <[email protected]>; Jack Christensen <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi Pavel,
On Wed, Mar 24, 2021 at 08:09:13AM +0100, Pavel Stehule wrote:
>
> I am continuing to talk with Hannu about syntax. I think so using the
> compile option is a good direction, but maybe renaming from "routine_label"
> to "topscope_label" (proposed by Hannu) or maybe "outerscope_label" can be
> a good idea.
It's not clear to me what is the status of this patch. You sent some updated
patch since with some typo fixes but I don't see any conclusion about the
direction this patch should go and/or how to name the new option.
Should the patch be reviewed or switched to Waiting on Author?
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: pl/pgsql feature request: shorthand for argument and local variable references
2022-01-06 07:02 Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
@ 2022-01-06 07:52 ` Pavel Stehule <[email protected]>
2022-01-06 09:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Pavel Stehule @ 2022-01-06 07:52 UTC (permalink / raw)
To: Julien Rouhaud <[email protected]>; +Cc: Michael Paquier <[email protected]>; Hannu Krosing <[email protected]>; Vik Fearing <[email protected]>; Chapman Flack <[email protected]>; Jack Christensen <[email protected]>; PostgreSQL Hackers <[email protected]>
čt 6. 1. 2022 v 8:02 odesílatel Julien Rouhaud <[email protected]> napsal:
> Hi Pavel,
>
> On Wed, Mar 24, 2021 at 08:09:13AM +0100, Pavel Stehule wrote:
> >
> > I am continuing to talk with Hannu about syntax. I think so using the
> > compile option is a good direction, but maybe renaming from
> "routine_label"
> > to "topscope_label" (proposed by Hannu) or maybe "outerscope_label" can
> be
> > a good idea.
>
> It's not clear to me what is the status of this patch. You sent some
> updated
> patch since with some typo fixes but I don't see any conclusion about the
> direction this patch should go and/or how to name the new option.
>
> Should the patch be reviewed or switched to Waiting on Author?
>
I am not sure if there is enough agreement and if there is enough necessity
for this feature.
In this discussion there were more general questions about future
development of plpgsql (about possible configurations and compiler
parametrizations, and how to switch the configuration on global and on
local levels). This is not fully solved yet. This is probably the reason
why discussion about this specific feature (and very simple feature) has
not reached a clear end. Generally, this is the same problem in Postgres.
Personally, I don't want to write a complicated patch for this relatively
trivial (and not too important feature). Sure, then it cannot solve more
general questions. I think the plpgsql option can work well - maybe with
the keyword "top_label" or "root_label". Probably some enhancing of the
DECLARE statement can work too (if it will be done well). But it requires
more work, because the DECLARE statement has block scope, and then the
alias of the top label has to have block scope too. Theoretically we can
propagate it to GUC too - same like plpgsql.variable_conflict. This simple
design can work (my opinion), but I understand so it is not a general reply
for people who can have more control over the behavior of plpgsql. But I
think it is a different issue and should be solved separately (and this
pretty complex problem, because Postgres together with PLpgSQL mix
different access models - access rights, searching path, local scope, and
we miss possible parametrization on schema level what has most sense for
PLpgSQL).
Regards
Pavel
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: pl/pgsql feature request: shorthand for argument and local variable references
2022-01-06 07:02 Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 07:52 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
@ 2022-01-06 09:05 ` Julien Rouhaud <[email protected]>
2022-01-06 13:28 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Joel Jacobson <[email protected]>
2022-03-05 08:54 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
0 siblings, 2 replies; 30+ messages in thread
From: Julien Rouhaud @ 2022-01-06 09:05 UTC (permalink / raw)
To: Pavel Stehule <[email protected]>; +Cc: Michael Paquier <[email protected]>; Hannu Krosing <[email protected]>; Vik Fearing <[email protected]>; Chapman Flack <[email protected]>; Jack Christensen <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Jan 06, 2022 at 08:52:23AM +0100, Pavel Stehule wrote:
>
> I am not sure if there is enough agreement and if there is enough necessity
> for this feature.
>
> In this discussion there were more general questions about future
> development of plpgsql (about possible configurations and compiler
> parametrizations, and how to switch the configuration on global and on
> local levels). This is not fully solved yet. This is probably the reason
> why discussion about this specific feature (and very simple feature) has
> not reached a clear end. Generally, this is the same problem in Postgres.
I agree, but on the other hand I don't see how defining a top level block
alias identical for every single plpgsql function would really make sense.
Not every function has a very long name and would benefit from it, and no one
can really assume that e.g. "root" or whatever configured name won't be used in
any plpgsql function on that database or cluster. So while having some global
configuration infrastructure can be useful I still don't think that it could be
used for this purpose.
Anyway, the only committer that showed some interest in the feature is Michael,
and he seemed ok in principle with the "alias-implementation" approach.
Michael, did you have a look at this version ([1]), or do you think it should
simply be rejected?
[1]: https://www.postgresql.org/message-id/attachment/120789/plpgsql-routine-label-option-alias-implement...
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: pl/pgsql feature request: shorthand for argument and local variable references
2022-01-06 07:02 Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 07:52 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 09:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
@ 2022-01-06 13:28 ` Joel Jacobson <[email protected]>
2022-01-06 14:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
1 sibling, 1 reply; 30+ messages in thread
From: Joel Jacobson @ 2022-01-06 13:28 UTC (permalink / raw)
To: [email protected]
On Thu, Jan 6, 2022, at 10:05, Julien Rouhaud wrote:
> I agree, but on the other hand I don't see how defining a top level block
> alias identical for every single plpgsql function would really make sense.
> Not every function has a very long name and would benefit from it, and no one
> can really assume that e.g. "root" or whatever configured name won't be used in
> any plpgsql function on that database or cluster. So while having some global
> configuration infrastructure can be useful I still don't think that it could be
> used for this purpose.
How about using the existing reserved keyword "in" followed by "." (dot) and then the function parameter name?
This idea is based on the assumption "in." would always be a syntax error everywhere in both SQL and PL/pgSQL,
so if we would introduce such a syntax, no existing code could be affected, except currently invalid code.
I wouldn't mind using "in." to refer to IN/OUT/INOUT parameters and not only IN ones, it's a minor confusion that could be explained in the docs.
Also, "out." wouldn't work, since "out" is not a reserved keyword.
/Joel
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: pl/pgsql feature request: shorthand for argument and local variable references
2022-01-06 07:02 Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 07:52 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 09:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 13:28 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Joel Jacobson <[email protected]>
@ 2022-01-06 14:05 ` Pavel Stehule <[email protected]>
2022-01-06 15:58 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Joel Jacobson <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Pavel Stehule @ 2022-01-06 14:05 UTC (permalink / raw)
To: Joel Jacobson <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
čt 6. 1. 2022 v 14:28 odesílatel Joel Jacobson <[email protected]> napsal:
> On Thu, Jan 6, 2022, at 10:05, Julien Rouhaud wrote:
> > I agree, but on the other hand I don't see how defining a top level block
> > alias identical for every single plpgsql function would really make
> sense.
> > Not every function has a very long name and would benefit from it, and
> no one
> > can really assume that e.g. "root" or whatever configured name won't be
> used in
> > any plpgsql function on that database or cluster. So while having some
> global
> > configuration infrastructure can be useful I still don't think that it
> could be
> > used for this purpose.
>
> How about using the existing reserved keyword "in" followed by "." (dot)
> and then the function parameter name?
>
> This idea is based on the assumption "in." would always be a syntax error
> everywhere in both SQL and PL/pgSQL,
> so if we would introduce such a syntax, no existing code could be
> affected, except currently invalid code.
>
> I wouldn't mind using "in." to refer to IN/OUT/INOUT parameters and not
> only IN ones, it's a minor confusion that could be explained in the docs.
>
You are right, in.outvar looks messy. Moreover, maybe the SQL parser can
have a problem with it.
Regards
Pavel
>
> Also, "out." wouldn't work, since "out" is not a reserved keyword.
>
> /Joel
>
>
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: pl/pgsql feature request: shorthand for argument and local variable references
2022-01-06 07:02 Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 07:52 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 09:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 13:28 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Joel Jacobson <[email protected]>
2022-01-06 14:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
@ 2022-01-06 15:58 ` Joel Jacobson <[email protected]>
2022-01-06 16:10 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Joel Jacobson @ 2022-01-06 15:58 UTC (permalink / raw)
To: Pavel Stehule <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Thu, Jan 6, 2022, at 15:05, Pavel Stehule wrote:
>>čt 6. 1. 2022 v 14:28 odesílatel Joel Jacobson <[email protected]> napsal:
>>How about using the existing reserved keyword "in" followed by "." (dot) and then the function parameter name?
>>
>>This idea is based on the assumption "in." would always be a syntax error everywhere in both SQL and PL/pgSQL,
>>so if we would introduce such a syntax, no existing code could be affected, except currently invalid code.
>>
>>I wouldn't mind using "in." to refer to IN/OUT/INOUT parameters and not only IN ones, it's a minor confusion that could be >>explained in the docs.
>
>You are right, in.outvar looks messy.
I think you misunderstood what I meant, I suggested "in.outvar" would actually be OK.
> Moreover, maybe the SQL parser can have a problem with it.
How could the SQL parser have a problem with it, if "in" is currently never followed by "." (dot)?
Not an expert in the SQL parser, trying to understand why it would be a problem.
/Joel
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: pl/pgsql feature request: shorthand for argument and local variable references
2022-01-06 07:02 Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 07:52 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 09:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 13:28 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Joel Jacobson <[email protected]>
2022-01-06 14:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 15:58 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Joel Jacobson <[email protected]>
@ 2022-01-06 16:10 ` Pavel Stehule <[email protected]>
2022-01-06 16:47 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Joel Jacobson <[email protected]>
2022-01-06 16:55 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Tom Lane <[email protected]>
0 siblings, 2 replies; 30+ messages in thread
From: Pavel Stehule @ 2022-01-06 16:10 UTC (permalink / raw)
To: Joel Jacobson <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
čt 6. 1. 2022 v 16:59 odesílatel Joel Jacobson <[email protected]> napsal:
> On Thu, Jan 6, 2022, at 15:05, Pavel Stehule wrote:
> >>čt 6. 1. 2022 v 14:28 odesílatel Joel Jacobson <[email protected]>
> napsal:
> >>How about using the existing reserved keyword "in" followed by "." (dot)
> and then the function parameter name?
> >>
> >>This idea is based on the assumption "in." would always be a syntax
> error everywhere in both SQL and PL/pgSQL,
> >>so if we would introduce such a syntax, no existing code could be
> affected, except currently invalid code.
> >>
> >>I wouldn't mind using "in." to refer to IN/OUT/INOUT parameters and not
> only IN ones, it's a minor confusion that could be >>explained in the docs.
> >
> >You are right, in.outvar looks messy.
>
> I think you misunderstood what I meant, I suggested "in.outvar" would
> actually be OK.
>
I understand well, and I don't think it's nice.
Are there some similar features in other programming languages?
> > Moreover, maybe the SQL parser can have a problem with it.
>
> How could the SQL parser have a problem with it, if "in" is currently
> never followed by "." (dot)?
> Not an expert in the SQL parser, trying to understand why it would be a
> problem.
>
you can check it. It is true, so IN is usually followed by "(", but until
check I am not able to say if there will be an unwanted shift or collision
or not.
Regards
Pavel
>
> /Joel
>
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: pl/pgsql feature request: shorthand for argument and local variable references
2022-01-06 07:02 Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 07:52 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 09:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 13:28 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Joel Jacobson <[email protected]>
2022-01-06 14:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 15:58 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Joel Jacobson <[email protected]>
2022-01-06 16:10 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
@ 2022-01-06 16:47 ` Joel Jacobson <[email protected]>
2022-01-06 18:03 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
1 sibling, 1 reply; 30+ messages in thread
From: Joel Jacobson @ 2022-01-06 16:47 UTC (permalink / raw)
To: Pavel Stehule <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Thu, Jan 6, 2022, at 17:10, Pavel Stehule wrote:
> I understand well, and I don't think it's nice.
>
> Are there some similar features in other programming languages?
It would be similar to "this" in Javascript/Java/C++,
but instead using "in" to access function parameters.
Currently, we need to prefix the parameter name if it's in conflict with a column name:
CREATE FUNCTION very_long_function_name(id int, some_value text)
RETURNS boolean
LANGUAGE sql AS $$
UPDATE some_table
SET some_value = very_long_function_name.some_value
WHERE id = very_long_function_name.id RETURNING TRUE
$$;
This is cumbersome as function names can be long, and if changing the function name,
you would need to update all occurrences of the function name in the code.
If we could instead refer to the parameters by prefixing them with "in.", we could write:
CREATE FUNCTION very_long_function_name(id int, some_value text)
RETURNS boolean
LANGUAGE sql AS $$
UPDATE some_table
SET some_value = in.some_value
WHERE id = in.id RETURNING TRUE
$$;
I think this would be nice, even if it would only work for IN parameters,
since you seldom need to access OUT parameters in the problematic WHERE-clauses anyway.
I mostly use OUT parameters when setting them on a separate row:
some_out_var := some_value;
...or, when SELECTin INTO an OUT parameter, which wouldn't be a problem.
> you can check it. It is true, so IN is usually followed by "(", but until check I am not able to say if there will be an unwanted
> shift or collision or not.
I checked gram.y, and IN_P is never followed by '.', but not sure if it could cause problems anyway, hope someone with parser knowledge can comment on this.
/Joel
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: pl/pgsql feature request: shorthand for argument and local variable references
2022-01-06 07:02 Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 07:52 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 09:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 13:28 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Joel Jacobson <[email protected]>
2022-01-06 14:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 15:58 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Joel Jacobson <[email protected]>
2022-01-06 16:10 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 16:47 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Joel Jacobson <[email protected]>
@ 2022-01-06 18:03 ` Pavel Stehule <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Pavel Stehule @ 2022-01-06 18:03 UTC (permalink / raw)
To: Joel Jacobson <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
čt 6. 1. 2022 v 17:48 odesílatel Joel Jacobson <[email protected]> napsal:
> On Thu, Jan 6, 2022, at 17:10, Pavel Stehule wrote:
> > I understand well, and I don't think it's nice.
> >
> > Are there some similar features in other programming languages?
>
> It would be similar to "this" in Javascript/Java/C++,
> but instead using "in" to access function parameters.
>
> Currently, we need to prefix the parameter name if it's in conflict with a
> column name:
>
> CREATE FUNCTION very_long_function_name(id int, some_value text)
> RETURNS boolean
> LANGUAGE sql AS $$
> UPDATE some_table
> SET some_value = very_long_function_name.some_value
> WHERE id = very_long_function_name.id RETURNING TRUE
> $$;
>
> This is cumbersome as function names can be long, and if changing the
> function name,
> you would need to update all occurrences of the function name in the code.
>
> If we could instead refer to the parameters by prefixing them with "in.",
> we could write:
>
> CREATE FUNCTION very_long_function_name(id int, some_value text)
> RETURNS boolean
> LANGUAGE sql AS $$
> UPDATE some_table
> SET some_value = in.some_value
> WHERE id = in.id RETURNING TRUE
> $$;
>
> I think this would be nice, even if it would only work for IN parameters,
> since you seldom need to access OUT parameters in the problematic
> WHERE-clauses anyway.
> I mostly use OUT parameters when setting them on a separate row:
> some_out_var := some_value;
> ...or, when SELECTin INTO an OUT parameter, which wouldn't be a problem.
>
There is full agreement in a description of the issue. Just I don't like
the proposed solution. The word "in '' is not too practical (where there
are out, and inout) - and it goes against the philosophy of ADA, where all
labels are parametrized (there is not any buildin label). The ADA language
has two things that plpgsql has not (unfortunately): a) possibility to
modify (configure) compiler by PRAGMA directive, b) possibility to define
PRAGMA on more levels - package, function, block. The possibility to define
a label dynamically is a better solution (not by some buildin keyword),
because it allows some possibility for the end user to define what he
prefers. For some cases "in" can be ok, but when you have only two out
variables, then "in" looks a little bit strange, and I prefer "fx", other
people can prefer "a" or "args".
There is no technical problem in the definition of alias of top namespace.
The problem is in syntax and in forcing this setting to some set of
routines. Theoretically we can have GUC plpgsql.rootns. I can set there
"fx", and you can set there "in" and we both can be happy. But the real
question is - how to force this setting to all functions. GUC can be
overwritten in session, and although you can set this GUC in every
function (by option or by assigned GUC), it is not nice, and somebody can
forget about this setting. For me, there are more interesting (important)
issues than the possibility to specify the root namespace that can be nice
to control. I miss some configuration mechanism independent of GUC that is
static (and that emulates static compile options), that can be assigned to
database (as synonym for project) or schema (as synonym for module or
package). With this mechanism this thread will be significantly shorter,
and all discussion about plpgsql2 was not.
>
> > you can check it. It is true, so IN is usually followed by "(", but
> until check I am not able to say if there will be an unwanted
> > shift or collision or not.
>
> I checked gram.y, and IN_P is never followed by '.', but not sure if it
> could cause problems anyway, hope someone with parser knowledge can comment
> on this.
>
> /Joel
>
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: pl/pgsql feature request: shorthand for argument and local variable references
2022-01-06 07:02 Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 07:52 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 09:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 13:28 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Joel Jacobson <[email protected]>
2022-01-06 14:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 15:58 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Joel Jacobson <[email protected]>
2022-01-06 16:10 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
@ 2022-01-06 16:55 ` Tom Lane <[email protected]>
1 sibling, 0 replies; 30+ messages in thread
From: Tom Lane @ 2022-01-06 16:55 UTC (permalink / raw)
To: Pavel Stehule <[email protected]>; +Cc: Joel Jacobson <[email protected]>; PostgreSQL Hackers <[email protected]>
Pavel Stehule <[email protected]> writes:
> čt 6. 1. 2022 v 16:59 odesílatel Joel Jacobson <[email protected]> napsal:
>> How could the SQL parser have a problem with it, if "in" is currently
>> never followed by "." (dot)?
> you can check it. It is true, so IN is usually followed by "(", but until
> check I am not able to say if there will be an unwanted shift or collision
> or not.
Even if it works today, we could be letting ourselves in for future
trouble. The SQL standard is a moving target, and they could easily
standardize some future syntax involving IN that creates a problem here.
I think we already have two perfectly satisfactory answers:
* qualify parameters with the function name to disambiguate them;
* use the ALIAS feature to create an internal, shorter name.
I see no problem here that is worth locking ourselves into unnecessary
syntax assumptions to fix.
regards, tom lane
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: pl/pgsql feature request: shorthand for argument and local variable references
2022-01-06 07:02 Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 07:52 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 09:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
@ 2022-03-05 08:54 ` Julien Rouhaud <[email protected]>
2022-03-05 10:31 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Michael Paquier <[email protected]>
1 sibling, 1 reply; 30+ messages in thread
From: Julien Rouhaud @ 2022-03-05 08:54 UTC (permalink / raw)
To: Pavel Stehule <[email protected]>; +Cc: Michael Paquier <[email protected]>; Hannu Krosing <[email protected]>; Vik Fearing <[email protected]>; Chapman Flack <[email protected]>; Jack Christensen <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Jan 06, 2022 at 05:05:32PM +0800, Julien Rouhaud wrote:
>
> Anyway, the only committer that showed some interest in the feature is Michael,
> and he seemed ok in principle with the "alias-implementation" approach.
> Michael, did you have a look at this version ([1]), or do you think it should
> simply be rejected?
After a couple of months I see that there is unfortunately no agreement on this
patch, or even its need.
I think we should close the patch as Rejected, and probably add an entry in the
"Feature we do not want" wiki page.
I will do that in a few days unless someone shows up with new arguments.
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: pl/pgsql feature request: shorthand for argument and local variable references
2022-01-06 07:02 Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 07:52 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 09:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-03-05 08:54 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
@ 2022-03-05 10:31 ` Michael Paquier <[email protected]>
2022-03-07 02:27 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Michael Paquier <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Michael Paquier @ 2022-03-05 10:31 UTC (permalink / raw)
To: Julien Rouhaud <[email protected]>; +Cc: Pavel Stehule <[email protected]>; Hannu Krosing <[email protected]>; Vik Fearing <[email protected]>; Chapman Flack <[email protected]>; Jack Christensen <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Mar 05, 2022 at 04:54:18PM +0800, Julien Rouhaud wrote:
> On Thu, Jan 06, 2022 at 05:05:32PM +0800, Julien Rouhaud wrote:
>> Anyway, the only committer that showed some interest in the feature is Michael,
>> and he seemed ok in principle with the "alias-implementation" approach.
>> Michael, did you have a look at this version ([1]), or do you think it should
>> simply be rejected?
>
> After a couple of months I see that there is unfortunately no agreement on this
> patch, or even its need.
I got a short look at what was proposed in the patch a couple of
months ago, and still found the implementation confusing with the way
aliases are handled, particularly when it came to several layers of
pl/pgsql. I am fine to let it go for now.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: pl/pgsql feature request: shorthand for argument and local variable references
2022-01-06 07:02 Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 07:52 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 09:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-03-05 08:54 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-03-05 10:31 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Michael Paquier <[email protected]>
@ 2022-03-07 02:27 ` Michael Paquier <[email protected]>
2022-03-07 02:31 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Michael Paquier @ 2022-03-07 02:27 UTC (permalink / raw)
To: Julien Rouhaud <[email protected]>; +Cc: Pavel Stehule <[email protected]>; Hannu Krosing <[email protected]>; Vik Fearing <[email protected]>; Chapman Flack <[email protected]>; Jack Christensen <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Mar 05, 2022 at 07:31:53PM +0900, Michael Paquier wrote:
> I got a short look at what was proposed in the patch a couple of
> months ago, and still found the implementation confusing with the way
> aliases are handled, particularly when it came to several layers of
> pl/pgsql. I am fine to let it go for now.
Just had an extra look at the patch, still same impression. So done
this way.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: pl/pgsql feature request: shorthand for argument and local variable references
2022-01-06 07:02 Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 07:52 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 09:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-03-05 08:54 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-03-05 10:31 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Michael Paquier <[email protected]>
2022-03-07 02:27 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Michael Paquier <[email protected]>
@ 2022-03-07 02:31 ` Julien Rouhaud <[email protected]>
2022-03-07 05:06 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Michael Paquier <[email protected]>
2022-03-07 05:35 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
0 siblings, 2 replies; 30+ messages in thread
From: Julien Rouhaud @ 2022-03-07 02:31 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Pavel Stehule <[email protected]>; Hannu Krosing <[email protected]>; Vik Fearing <[email protected]>; Chapman Flack <[email protected]>; Jack Christensen <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, Mar 07, 2022 at 11:27:14AM +0900, Michael Paquier wrote:
> On Sat, Mar 05, 2022 at 07:31:53PM +0900, Michael Paquier wrote:
> > I got a short look at what was proposed in the patch a couple of
> > months ago, and still found the implementation confusing with the way
> > aliases are handled, particularly when it came to several layers of
> > pl/pgsql. I am fine to let it go for now.
>
> Just had an extra look at the patch, still same impression. So done
> this way.
I was actually waiting a bit to make sure that Pavel could read the thread,
since it was the weekend and right now it's 3:30 AM in Czech Republic...
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: pl/pgsql feature request: shorthand for argument and local variable references
2022-01-06 07:02 Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 07:52 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 09:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-03-05 08:54 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-03-05 10:31 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Michael Paquier <[email protected]>
2022-03-07 02:27 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Michael Paquier <[email protected]>
2022-03-07 02:31 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
@ 2022-03-07 05:06 ` Michael Paquier <[email protected]>
1 sibling, 0 replies; 30+ messages in thread
From: Michael Paquier @ 2022-03-07 05:06 UTC (permalink / raw)
To: Julien Rouhaud <[email protected]>; +Cc: Pavel Stehule <[email protected]>; Hannu Krosing <[email protected]>; Vik Fearing <[email protected]>; Chapman Flack <[email protected]>; Jack Christensen <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, Mar 07, 2022 at 10:31:40AM +0800, Julien Rouhaud wrote:
> I was actually waiting a bit to make sure that Pavel could read the thread,
> since it was the weekend and right now it's 3:30 AM in Czech Republic...
Sorry about that. I have reset the state of the patch.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: pl/pgsql feature request: shorthand for argument and local variable references
2022-01-06 07:02 Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 07:52 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 09:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-03-05 08:54 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-03-05 10:31 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Michael Paquier <[email protected]>
2022-03-07 02:27 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Michael Paquier <[email protected]>
2022-03-07 02:31 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
@ 2022-03-07 05:35 ` Pavel Stehule <[email protected]>
2022-03-07 05:48 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
1 sibling, 1 reply; 30+ messages in thread
From: Pavel Stehule @ 2022-03-07 05:35 UTC (permalink / raw)
To: Julien Rouhaud <[email protected]>; +Cc: Michael Paquier <[email protected]>; Hannu Krosing <[email protected]>; Vik Fearing <[email protected]>; Chapman Flack <[email protected]>; Jack Christensen <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi
po 7. 3. 2022 v 3:31 odesílatel Julien Rouhaud <[email protected]> napsal:
> On Mon, Mar 07, 2022 at 11:27:14AM +0900, Michael Paquier wrote:
> > On Sat, Mar 05, 2022 at 07:31:53PM +0900, Michael Paquier wrote:
> > > I got a short look at what was proposed in the patch a couple of
> > > months ago, and still found the implementation confusing with the way
> > > aliases are handled, particularly when it came to several layers of
> > > pl/pgsql. I am fine to let it go for now.
> >
> > Just had an extra look at the patch, still same impression. So done
> > this way.
>
> I was actually waiting a bit to make sure that Pavel could read the
> thread,
> since it was the weekend and right now it's 3:30 AM in Czech Republic...
>
this patch should be rejected. There is no consensus.
Regards
Pavel
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: pl/pgsql feature request: shorthand for argument and local variable references
2022-01-06 07:02 Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 07:52 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
2022-01-06 09:05 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-03-05 08:54 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-03-05 10:31 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Michael Paquier <[email protected]>
2022-03-07 02:27 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Michael Paquier <[email protected]>
2022-03-07 02:31 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-03-07 05:35 ` Re: pl/pgsql feature request: shorthand for argument and local variable references Pavel Stehule <[email protected]>
@ 2022-03-07 05:48 ` Julien Rouhaud <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Julien Rouhaud @ 2022-03-07 05:48 UTC (permalink / raw)
To: Pavel Stehule <[email protected]>; +Cc: Michael Paquier <[email protected]>; Hannu Krosing <[email protected]>; Vik Fearing <[email protected]>; Chapman Flack <[email protected]>; Jack Christensen <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, Mar 07, 2022 at 06:35:45AM +0100, Pavel Stehule wrote:
>
> this patch should be rejected. There is no consensus.
Thanks for the confirmation, I will take care of it!
^ permalink raw reply [nested|flat] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ 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; 30+ 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] 30+ messages in thread
end of thread, other threads:[~2024-03-25 23:39 UTC | newest]
Thread overview: 30+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-01-06 07:02 Re: pl/pgsql feature request: shorthand for argument and local variable references Julien Rouhaud <[email protected]>
2022-01-06 07:52 ` Pavel Stehule <[email protected]>
2022-01-06 09:05 ` Julien Rouhaud <[email protected]>
2022-01-06 13:28 ` Joel Jacobson <[email protected]>
2022-01-06 14:05 ` Pavel Stehule <[email protected]>
2022-01-06 15:58 ` Joel Jacobson <[email protected]>
2022-01-06 16:10 ` Pavel Stehule <[email protected]>
2022-01-06 16:47 ` Joel Jacobson <[email protected]>
2022-01-06 18:03 ` Pavel Stehule <[email protected]>
2022-01-06 16:55 ` Tom Lane <[email protected]>
2022-03-05 08:54 ` Julien Rouhaud <[email protected]>
2022-03-05 10:31 ` Michael Paquier <[email protected]>
2022-03-07 02:27 ` Michael Paquier <[email protected]>
2022-03-07 02:31 ` Julien Rouhaud <[email protected]>
2022-03-07 05:06 ` Michael Paquier <[email protected]>
2022-03-07 05:35 ` Pavel Stehule <[email protected]>
2022-03-07 05:48 ` Julien Rouhaud <[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 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 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-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 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]>
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