public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v4 11/19] Separate tuple pre freeze checks and invoke earlier
7+ messages / 4 participants
[nested] [flat]
* [PATCH v4 11/19] Separate tuple pre freeze checks and invoke earlier
@ 2024-01-07 21:53 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Melanie Plageman @ 2024-01-07 21:53 UTC (permalink / raw)
When combining the prune and freeze records their critical sections will
have to be combined. heap_freeze_execute_prepared() does a set of pre
freeze validations before starting its critical section. Move these
validations into a helper function, heap_pre_freeze_checks(), and invoke
it in heap_page_prune() before the pruning critical section.
Also move up the calculation of the freeze snapshot conflict horizon.
---
src/backend/access/heap/heapam.c | 58 ++++++++++++++++-------------
src/backend/access/heap/pruneheap.c | 31 ++++++++-------
src/include/access/heapam.h | 3 ++
3 files changed, 54 insertions(+), 38 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 7261c4988d7..16e3f2520a4 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6659,35 +6659,19 @@ heap_execute_freeze_tuple(HeapTupleHeader tuple, HeapTupleFreeze *frz)
}
/*
- * heap_freeze_execute_prepared
- *
- * Executes freezing of one or more heap tuples on a page on behalf of caller.
- * Caller passes an array of tuple plans from heap_prepare_freeze_tuple.
- * Caller must set 'offset' in each plan for us. Note that we destructively
- * sort caller's tuples array in-place, so caller had better be done with it.
- *
- * WAL-logs the changes so that VACUUM can advance the rel's relfrozenxid
- * later on without any risk of unsafe pg_xact lookups, even following a hard
- * crash (or when querying from a standby). We represent freezing by setting
- * infomask bits in tuple headers, but this shouldn't be thought of as a hint.
- * See section on buffer access rules in src/backend/storage/buffer/README.
- */
+* Perform xmin/xmax XID status sanity checks before calling
+* heap_freeze_execute_prepared().
+*
+* heap_prepare_freeze_tuple doesn't perform these checks directly because
+* pg_xact lookups are relatively expensive. They shouldn't be repeated
+* by successive VACUUMs that each decide against freezing the same page.
+*/
void
-heap_freeze_execute_prepared(Relation rel, Buffer buffer,
- TransactionId snapshotConflictHorizon,
- HeapTupleFreeze *tuples, int ntuples)
+heap_pre_freeze_checks(Buffer buffer,
+ HeapTupleFreeze *tuples, int ntuples)
{
Page page = BufferGetPage(buffer);
- Assert(ntuples > 0);
-
- /*
- * Perform xmin/xmax XID status sanity checks before critical section.
- *
- * heap_prepare_freeze_tuple doesn't perform these checks directly because
- * pg_xact lookups are relatively expensive. They shouldn't be repeated
- * by successive VACUUMs that each decide against freezing the same page.
- */
for (int i = 0; i < ntuples; i++)
{
HeapTupleFreeze *frz = tuples + i;
@@ -6726,6 +6710,30 @@ heap_freeze_execute_prepared(Relation rel, Buffer buffer,
xmax)));
}
}
+}
+
+/*
+ * heap_freeze_execute_prepared
+ *
+ * Executes freezing of one or more heap tuples on a page on behalf of caller.
+ * Caller passes an array of tuple plans from heap_prepare_freeze_tuple.
+ * Caller must set 'offset' in each plan for us. Note that we destructively
+ * sort caller's tuples array in-place, so caller had better be done with it.
+ *
+ * WAL-logs the changes so that VACUUM can advance the rel's relfrozenxid
+ * later on without any risk of unsafe pg_xact lookups, even following a hard
+ * crash (or when querying from a standby). We represent freezing by setting
+ * infomask bits in tuple headers, but this shouldn't be thought of as a hint.
+ * See section on buffer access rules in src/backend/storage/buffer/README.
+ */
+void
+heap_freeze_execute_prepared(Relation rel, Buffer buffer,
+ TransactionId snapshotConflictHorizon,
+ HeapTupleFreeze *tuples, int ntuples)
+{
+ Page page = BufferGetPage(buffer);
+
+ Assert(ntuples > 0);
START_CRIT_SECTION();
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index 9edf6bf72d7..87f99497865 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -524,6 +524,24 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
(pagefrz->freeze_required ||
(whole_page_freezable && presult->nfrozen > 0 && (prune_fpi || hint_bit_fpi)));
+ if (do_freeze)
+ {
+ heap_pre_freeze_checks(buffer, prstate.frozen, presult->nfrozen);
+
+ /*
+ * 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))
+ {
+ /* Avoids false conflicts when hot_standby_feedback in use */
+ presult->frz_conflict_horizon = pagefrz->cutoffs->OldestXmin;
+ TransactionIdRetreat(presult->frz_conflict_horizon);
+ }
+ }
+
/* Any error while applying the changes is critical */
START_CRIT_SECTION();
@@ -622,19 +640,6 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer,
if (do_freeze)
{
- /*
- * 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))
- {
- /* Avoids false conflicts when hot_standby_feedback in use */
- presult->frz_conflict_horizon = pagefrz->cutoffs->OldestXmin;
- TransactionIdRetreat(presult->frz_conflict_horizon);
- }
-
/* Execute all freeze plans for page as a single atomic action */
heap_freeze_execute_prepared(relation, buffer,
presult->frz_conflict_horizon,
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index b2a4caeb33a..02e33f213e1 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -312,6 +312,9 @@ extern void heap_inplace_update(Relation relation, HeapTuple tuple);
extern bool heap_prepare_freeze_tuple(HeapTupleHeader tuple,
HeapPageFreeze *pagefrz,
HeapTupleFreeze *frz, bool *totally_frozen);
+
+extern void heap_pre_freeze_checks(Buffer buffer,
+ HeapTupleFreeze *tuples, int ntuples);
extern void heap_freeze_execute_prepared(Relation rel, Buffer buffer,
TransactionId snapshotConflictHorizon,
HeapTupleFreeze *tuples, int ntuples);
--
2.40.1
--tez7m2a73jtztiij
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v4-0012-Remove-heap_freeze_execute_prepared.patch"
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Add more SQL/JSON constructor functions
@ 2024-05-29 16:44 Tom Lane <[email protected]>
2024-06-02 21:17 ` Re: pgsql: Add more SQL/JSON constructor functions Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2024-05-29 16:44 UTC (permalink / raw)
To: Amit Langote <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; [email protected]
Amit Langote <[email protected]> writes:
> On Mon, May 27, 2024 at 7:10 PM Alvaro Herrera <[email protected]> wrote:
>> On 2024-May-27, Alvaro Herrera wrote:
>> I just noticed this behavior, which looks like a bug to me:
>>
>> select json_serialize('{"a":1, "a":2}' returning varchar(5));
>> json_serialize
>> ────────────────
>> {"a":
>>
>> I think this function should throw an error if the destination type
>> doesn't have room for the output json. Otherwise, what good is the
>> serialization function?
> This behavior comes from using COERCE_EXPLICIT_CAST when creating the
> coercion expression to convert json_*() functions' argument to the
> RETURNING type.
Yeah, I too think this is a cast, and truncation is the spec-defined
behavior for casting to varchar with a specific length limit. I see
little reason that this should work differently from
select json_serialize('{"a":1, "a":2}' returning text)::varchar(5);
json_serialize
----------------
{"a":
(1 row)
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Add more SQL/JSON constructor functions
2024-05-29 16:44 Re: pgsql: Add more SQL/JSON constructor functions Tom Lane <[email protected]>
@ 2024-06-02 21:17 ` Peter Eisentraut <[email protected]>
2024-06-03 04:46 ` Re: pgsql: Add more SQL/JSON constructor functions Tom Lane <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Peter Eisentraut @ 2024-06-02 21:17 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Amit Langote <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; [email protected]
On 29.05.24 18:44, Tom Lane wrote:
> Amit Langote <[email protected]> writes:
>> On Mon, May 27, 2024 at 7:10 PM Alvaro Herrera <[email protected]> wrote:
>>> On 2024-May-27, Alvaro Herrera wrote:
>>> I just noticed this behavior, which looks like a bug to me:
>>>
>>> select json_serialize('{"a":1, "a":2}' returning varchar(5));
>>> json_serialize
>>> ────────────────
>>> {"a":
>>>
>>> I think this function should throw an error if the destination type
>>> doesn't have room for the output json. Otherwise, what good is the
>>> serialization function?
>
>> This behavior comes from using COERCE_EXPLICIT_CAST when creating the
>> coercion expression to convert json_*() functions' argument to the
>> RETURNING type.
>
> Yeah, I too think this is a cast, and truncation is the spec-defined
> behavior for casting to varchar with a specific length limit. I see
> little reason that this should work differently from
>
> select json_serialize('{"a":1, "a":2}' returning text)::varchar(5);
> json_serialize
> ----------------
> {"a":
> (1 row)
The SQL standard says essentially that the output of json_serialize() is
some string that when parsed back in gives you an equivalent JSON value
as the input. That doesn't seem compatible with truncating the output.
If you want output truncation, you can of course use an actual cast.
But it makes sense that the RETURNING clause is separate from that.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Add more SQL/JSON constructor functions
2024-05-29 16:44 Re: pgsql: Add more SQL/JSON constructor functions Tom Lane <[email protected]>
2024-06-02 21:17 ` Re: pgsql: Add more SQL/JSON constructor functions Peter Eisentraut <[email protected]>
@ 2024-06-03 04:46 ` Tom Lane <[email protected]>
2024-06-03 09:15 ` Re: pgsql: Add more SQL/JSON constructor functions Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2024-06-03 04:46 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Amit Langote <[email protected]>; Alvaro Herrera <[email protected]>; [email protected]
Peter Eisentraut <[email protected]> writes:
> On 29.05.24 18:44, Tom Lane wrote:
>> Yeah, I too think this is a cast, and truncation is the spec-defined
>> behavior for casting to varchar with a specific length limit.
> The SQL standard says essentially that the output of json_serialize() is
> some string that when parsed back in gives you an equivalent JSON value
> as the input. That doesn't seem compatible with truncating the output.
Maybe you should take this up with the SQL committee? If you don't
like our current behavior, then either you have to say that RETURNING
with a length-limited target type is illegal (which is problematic
for the spec, since they have no such type) or that the cast behaves
like an implicit cast, with errors for overlength input (which I find
to be an unintuitive definition for a construct that names the target
type explicitly).
> If you want output truncation, you can of course use an actual cast.
> But it makes sense that the RETURNING clause is separate from that.
Are you trying to say that the RETURNING clause's specified type
isn't the actual output type? I can't buy that either.
Again, if you think our existing behavior isn't right, I think
it's a problem for the SQL committee not us.
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Add more SQL/JSON constructor functions
2024-05-29 16:44 Re: pgsql: Add more SQL/JSON constructor functions Tom Lane <[email protected]>
2024-06-02 21:17 ` Re: pgsql: Add more SQL/JSON constructor functions Peter Eisentraut <[email protected]>
2024-06-03 04:46 ` Re: pgsql: Add more SQL/JSON constructor functions Tom Lane <[email protected]>
@ 2024-06-03 09:15 ` Peter Eisentraut <[email protected]>
2024-06-03 17:20 ` Re: pgsql: Add more SQL/JSON constructor functions Tom Lane <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Peter Eisentraut @ 2024-06-03 09:15 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Amit Langote <[email protected]>; Alvaro Herrera <[email protected]>; [email protected]
On 02.06.24 21:46, Tom Lane wrote:
> If you don't
> like our current behavior, then either you have to say that RETURNING
> with a length-limited target type is illegal (which is problematic
> for the spec, since they have no such type) or that the cast behaves
> like an implicit cast, with errors for overlength input (which I find
> to be an unintuitive definition for a construct that names the target
> type explicitly).
It asks for the latter behavior, essentially (but it's not defined in
terms of casts). It says:
"""
ii) Let JV be an implementation-dependent (UV097) value of type TT and
encoding ENC such that these two conditions hold:
1) JV is a JSON text.
2) When the General Rules of Subclause 9.42, “Parsing JSON text”, are
applied with JV as JSON TEXT, FO as FORMAT OPTION, and WITHOUT UNIQUE
KEYS as UNIQUENESS CONSTRAINT; let CST be the STATUS and let CSJI be the
SQL/JSON ITEM returned from the application of those General Rules, CST
is successful completion (00000) and CSJI is an SQL/JSON item that is
equivalent to SJI.
If there is no such JV, then let ST be the exception condition: data
exception — invalid JSON text (22032).
iii) If JV is longer than the length or maximum length of TT, then an
exception condition is raised: data exception — string data, right
truncation (22001).
"""
Oracle also behaves accordingly:
SQL> select json_serialize('{"a":1, "a":2}' returning varchar2(20)) from
dual;
JSON_SERIALIZE('{"A"
--------------------
{"a":1,"a":2}
SQL> select json_serialize('{"a":1, "a":2}' returning varchar2(5)) from
dual;
select json_serialize('{"a":1, "a":2}' returning varchar2(5)) from dual
*
ERROR at line 1:
ORA-40478: output value too large (maximum: 5)
JZN-00018: Input to serializer is too large
Help: https://docs.oracle.com/error-help/db/ora-40478/
As opposed to:
SQL> select cast(json_serialize('{"a":1, "a":2}') as varchar2(5)) from dual;
CAST(
-----
{"a":
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Add more SQL/JSON constructor functions
2024-05-29 16:44 Re: pgsql: Add more SQL/JSON constructor functions Tom Lane <[email protected]>
2024-06-02 21:17 ` Re: pgsql: Add more SQL/JSON constructor functions Peter Eisentraut <[email protected]>
2024-06-03 04:46 ` Re: pgsql: Add more SQL/JSON constructor functions Tom Lane <[email protected]>
2024-06-03 09:15 ` Re: pgsql: Add more SQL/JSON constructor functions Peter Eisentraut <[email protected]>
@ 2024-06-03 17:20 ` Tom Lane <[email protected]>
2024-06-04 10:03 ` Re: pgsql: Add more SQL/JSON constructor functions Amit Langote <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2024-06-03 17:20 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Amit Langote <[email protected]>; Alvaro Herrera <[email protected]>; [email protected]
Peter Eisentraut <[email protected]> writes:
> On 02.06.24 21:46, Tom Lane wrote:
>> If you don't
>> like our current behavior, then either you have to say that RETURNING
>> with a length-limited target type is illegal (which is problematic
>> for the spec, since they have no such type) or that the cast behaves
>> like an implicit cast, with errors for overlength input (which I find
>> to be an unintuitive definition for a construct that names the target
>> type explicitly).
> It asks for the latter behavior, essentially (but it's not defined in
> terms of casts). It says:
Meh. Who needs consistency? But I guess the answer is to do what was
suggested earlier and change the code to use COERCE_IMPLICIT_CAST.
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pgsql: Add more SQL/JSON constructor functions
2024-05-29 16:44 Re: pgsql: Add more SQL/JSON constructor functions Tom Lane <[email protected]>
2024-06-02 21:17 ` Re: pgsql: Add more SQL/JSON constructor functions Peter Eisentraut <[email protected]>
2024-06-03 04:46 ` Re: pgsql: Add more SQL/JSON constructor functions Tom Lane <[email protected]>
2024-06-03 09:15 ` Re: pgsql: Add more SQL/JSON constructor functions Peter Eisentraut <[email protected]>
2024-06-03 17:20 ` Re: pgsql: Add more SQL/JSON constructor functions Tom Lane <[email protected]>
@ 2024-06-04 10:03 ` Amit Langote <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Amit Langote @ 2024-06-04 10:03 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Alvaro Herrera <[email protected]>; [email protected]
On Tue, Jun 4, 2024 at 2:20 AM Tom Lane <[email protected]> wrote:
> Peter Eisentraut <[email protected]> writes:
> > On 02.06.24 21:46, Tom Lane wrote:
> >> If you don't
> >> like our current behavior, then either you have to say that RETURNING
> >> with a length-limited target type is illegal (which is problematic
> >> for the spec, since they have no such type) or that the cast behaves
> >> like an implicit cast, with errors for overlength input (which I find
> >> to be an unintuitive definition for a construct that names the target
> >> type explicitly).
>
> > It asks for the latter behavior, essentially (but it's not defined in
> > terms of casts). It says:
>
> Meh. Who needs consistency? But I guess the answer is to do what was
> suggested earlier and change the code to use COERCE_IMPLICIT_CAST.
OK, will post a patch to do so in a new thread on -hackers.
--
Thanks, Amit Langote
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2024-06-04 10:03 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-01-07 21:53 [PATCH v4 11/19] Separate tuple pre freeze checks and invoke earlier Melanie Plageman <[email protected]>
2024-05-29 16:44 Re: pgsql: Add more SQL/JSON constructor functions Tom Lane <[email protected]>
2024-06-02 21:17 ` Re: pgsql: Add more SQL/JSON constructor functions Peter Eisentraut <[email protected]>
2024-06-03 04:46 ` Re: pgsql: Add more SQL/JSON constructor functions Tom Lane <[email protected]>
2024-06-03 09:15 ` Re: pgsql: Add more SQL/JSON constructor functions Peter Eisentraut <[email protected]>
2024-06-03 17:20 ` Re: pgsql: Add more SQL/JSON constructor functions Tom Lane <[email protected]>
2024-06-04 10:03 ` Re: pgsql: Add more SQL/JSON constructor functions Amit Langote <[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