public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 2/8] Expand the external data before forming the tuple
6+ messages / 5 participants
[nested] [flat]
* [PATCH 2/8] Expand the external data before forming the tuple
@ 2021-03-04 09:25 Dilip Kumar <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Dilip Kumar @ 2021-03-04 09:25 UTC (permalink / raw)
All the callers of HeapTupleGetDatum and HeapTupleHeaderGetDatum, who might
contain the external varlena in their tuple, try to flatten the external
varlena before forming the tuple wherever it is possible.
Dilip Kumar based on idea by Robert Haas
---
src/backend/executor/execExprInterp.c | 29 +++++++++++++++++++++++++--
src/backend/utils/adt/jsonfuncs.c | 8 ++++++--
src/pl/plpgsql/src/pl_exec.c | 5 ++++-
3 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 20949a5dec..c3754acca4 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -2840,12 +2840,25 @@ ExecEvalRow(ExprState *state, ExprEvalStep *op)
{
HeapTuple tuple;
+ /* Retrieve externally stored values and decompress. */
+ for (int i = 0; i < op->d.row.tupdesc->natts; i++)
+ {
+ Form_pg_attribute attr = TupleDescAttr(op->d.row.tupdesc, i);
+
+ if (op->d.row.elemnulls[i] || attr->attlen != -1 ||
+ !VARATT_IS_EXTERNAL(DatumGetPointer(op->d.row.elemvalues[i])))
+ continue;
+
+ op->d.row.elemvalues[i] =
+ PointerGetDatum(PG_DETOAST_DATUM_PACKED(op->d.row.elemvalues[i]));
+ }
+
/* build tuple from evaluated field values */
tuple = heap_form_tuple(op->d.row.tupdesc,
op->d.row.elemvalues,
op->d.row.elemnulls);
- *op->resvalue = HeapTupleGetDatum(tuple);
+ *op->resvalue = HeapTupleGetRawDatum(tuple);
*op->resnull = false;
}
@@ -3085,12 +3098,24 @@ ExecEvalFieldStoreForm(ExprState *state, ExprEvalStep *op, ExprContext *econtext
{
HeapTuple tuple;
+ /* Retrieve externally stored values and decompress. */
+ for (int i = 0; i < (*op->d.fieldstore.argdesc)->natts; i++)
+ {
+ Form_pg_attribute attr = TupleDescAttr(*op->d.fieldstore.argdesc, i);
+
+ if (op->d.fieldstore.nulls[i] || attr->attlen != -1 ||
+ !VARATT_IS_EXTERNAL(DatumGetPointer(op->d.fieldstore.values[i])))
+ continue;
+ op->d.fieldstore.values[i] = PointerGetDatum(
+ PG_DETOAST_DATUM_PACKED(op->d.fieldstore.values[i]));
+ }
+
/* argdesc should already be valid from the DeForm step */
tuple = heap_form_tuple(*op->d.fieldstore.argdesc,
op->d.fieldstore.values,
op->d.fieldstore.nulls);
- *op->resvalue = HeapTupleGetDatum(tuple);
+ *op->resvalue = HeapTupleGetRawDatum(tuple);
*op->resnull = false;
}
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 511467280f..c3d464f42b 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -2968,7 +2968,7 @@ populate_composite(CompositeIOData *io,
/* populate resulting record tuple */
tuple = populate_record(io->tupdesc, &io->record_io,
defaultval, mcxt, &jso);
- result = HeapTupleHeaderGetDatum(tuple);
+ result = HeapTupleHeaderGetRawDatum(tuple);
JsObjectFree(&jso);
}
@@ -3387,6 +3387,10 @@ populate_record(TupleDesc tupdesc,
nulls[i] ? (Datum) 0 : values[i],
&field,
&nulls[i]);
+
+ if (!nulls[i] && att->attlen == -1 &&
+ VARATT_IS_EXTERNAL(DatumGetPointer(values[i])))
+ values[i] = PointerGetDatum(PG_DETOAST_DATUM_PACKED(values[i]));
}
res = heap_form_tuple(tupdesc, values, nulls);
@@ -3765,7 +3769,7 @@ populate_recordset_record(PopulateRecordsetState *state, JsObject *obj)
/* if it's domain over composite, check domain constraints */
if (cache->c.typcat == TYPECAT_COMPOSITE_DOMAIN)
- domain_check(HeapTupleHeaderGetDatum(tuphead), false,
+ domain_check(HeapTupleHeaderGetRawDatum(tuphead), false,
cache->argtype,
&cache->c.io.composite.domain_info,
cache->fn_mcxt);
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index b4c70aaa7f..fd073767bc 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -5326,7 +5326,7 @@ exec_eval_datum(PLpgSQL_execstate *estate,
elog(ERROR, "row not compatible with its own tupdesc");
*typeid = row->rowtupdesc->tdtypeid;
*typetypmod = row->rowtupdesc->tdtypmod;
- *value = HeapTupleGetDatum(tup);
+ *value = HeapTupleGetRawDatum(tup);
*isnull = false;
MemoryContextSwitchTo(oldcontext);
break;
@@ -7300,6 +7300,9 @@ make_tuple_from_row(PLpgSQL_execstate *estate,
&dvalues[i], &nulls[i]);
if (fieldtypeid != TupleDescAttr(tupdesc, i)->atttypid)
return NULL;
+ if (!nulls[i] && TupleDescAttr(tupdesc, i)->attlen == -1 &&
+ VARATT_IS_EXTERNAL(DatumGetPointer(dvalues[i])))
+ dvalues[i] = PointerGetDatum(PG_DETOAST_DATUM_PACKED(dvalues[i]));
/* XXX should we insist on typmod match, too? */
}
--
2.17.0
--cvVnyQ+4j833TQvp
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0003-Disallow-compressed-data-inside-container-types.patch"
^ permalink raw reply [nested|flat] 6+ messages in thread
* Design of pg_stat_subscription_workers vs pgstats
@ 2022-01-25 06:31 Andres Freund <[email protected]>
2022-01-25 11:27 ` Re: Design of pg_stat_subscription_workers vs pgstats Masahiko Sawada <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Andres Freund @ 2022-01-25 06:31 UTC (permalink / raw)
To: pgsql-hackers; +Cc: Masahiko Sawada <[email protected]>; Amit Kapila <[email protected]>
Hi,
I was looking the shared memory stats patch again. The rebase of which
collided fairly heavily with the addition of pg_stat_subscription_workers.
I'm concerned about the design of pg_stat_subscription_workers. The view was
introduced in
commit 8d74fc96db5fd547e077bf9bf4c3b67f821d71cd
Author: Amit Kapila <[email protected]>
Date: 2021-11-30 08:54:30 +0530
Add a view to show the stats of subscription workers.
This commit adds a new system view pg_stat_subscription_workers, that
shows information about any errors which occur during the application of
logical replication changes as well as during performing initial table
synchronization. The subscription statistics entries are removed when the
corresponding subscription is removed.
It also adds an SQL function pg_stat_reset_subscription_worker() to reset
single subscription errors.
The contents of this view can be used by an upcoming patch that skips the
particular transaction that conflicts with the existing data on the
subscriber.
This view can be extended in the future to track other xact related
statistics like the number of xacts committed/aborted for subscription
workers.
Author: Masahiko Sawada
Reviewed-by: Greg Nancarrow, Hou Zhijie, Tang Haiying, Vignesh C, Dilip Kumar, Takamichi Osumi, Amit Kapila
Discussion: https://postgr.es/m/CAD21AoDeScrsHhLyEPYqN3sydg6PxAPVBboK=30xJfUVihNZDA@mail.gmail.com
I tried to skim-read the discussion leading to its introduction, but it's
extraordinarily long: 474 messages in [1], 131 messages in [2], as well as a
few other associated threads.
From the commit message alone I am concerned that this appears to be intended
to be used to store important state in pgstats. For which pgstats is
fundamentally unsuitable (pgstat can loose state during normal operation,
always looses state during crash restarts, the state can be reset).
I don't really understand the name "pg_stat_subscription_workers" - what
workers are stats kept about exactly? The columns don't obviously refer to a
single worker or such? From the contents it should be name
pg_stat_subscription_table_stats or such. But no, that'd not quite right,
because apply errors are stored per-susbcription, while initial sync stuff is
per-(subscription, table).
The pgstat entries are quite wide (292 bytes), because of the error message
stored. That's nearly twice the size of PgStat_StatTabEntry. And as far as I
can tell, once there was an error, we'll just keep the stats entry around
until the subscription is dropped. And that includes stats for long dropped
tables, as far as I can see - except that they're hidden from view, due to a
join to pg_subscription_rel.
To me this looks like it's using pgstat as an extremely poor IPC mechanism.
Why isn't this just storing data in pg_subscription_rel?
Greetings,
Andres Freund
[1] https://www.postgresql.org/message-id/CAD21AoDeScrsHhLyEPYqN3sydg6PxAPVBboK%3D30xJfUVihNZDA%40mail.g...
[2] https://postgr.es/m/OSBPR01MB48887CA8F40C8D984A6DC00CED199%40OSBPR01MB4888.jpnprd01.prod.outlook.com
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats
2022-01-25 06:31 Design of pg_stat_subscription_workers vs pgstats Andres Freund <[email protected]>
@ 2022-01-25 11:27 ` Masahiko Sawada <[email protected]>
2022-01-27 05:46 ` Re: Design of pg_stat_subscription_workers vs pgstats Andres Freund <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Masahiko Sawada @ 2022-01-25 11:27 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: pgsql-hackers; Amit Kapila <[email protected]>
Hi,
On Tue, Jan 25, 2022 at 3:31 PM Andres Freund <[email protected]> wrote:
>
> Hi,
>
> I was looking the shared memory stats patch again. The rebase of which
> collided fairly heavily with the addition of pg_stat_subscription_workers.
>
> I'm concerned about the design of pg_stat_subscription_workers. The view was
> introduced in
>
>
> commit 8d74fc96db5fd547e077bf9bf4c3b67f821d71cd
> Author: Amit Kapila <[email protected]>
> Date: 2021-11-30 08:54:30 +0530
>
> Add a view to show the stats of subscription workers.
>
> This commit adds a new system view pg_stat_subscription_workers, that
> shows information about any errors which occur during the application of
> logical replication changes as well as during performing initial table
> synchronization. The subscription statistics entries are removed when the
> corresponding subscription is removed.
>
> It also adds an SQL function pg_stat_reset_subscription_worker() to reset
> single subscription errors.
>
> The contents of this view can be used by an upcoming patch that skips the
> particular transaction that conflicts with the existing data on the
> subscriber.
>
> This view can be extended in the future to track other xact related
> statistics like the number of xacts committed/aborted for subscription
> workers.
>
> Author: Masahiko Sawada
> Reviewed-by: Greg Nancarrow, Hou Zhijie, Tang Haiying, Vignesh C, Dilip Kumar, Takamichi Osumi, Amit Kapila
> Discussion: https://postgr.es/m/CAD21AoDeScrsHhLyEPYqN3sydg6PxAPVBboK=30xJfUVihNZDA@mail.gmail.com
>
>
> I tried to skim-read the discussion leading to its introduction, but it's
> extraordinarily long: 474 messages in [1], 131 messages in [2], as well as a
> few other associated threads.
>
>
> From the commit message alone I am concerned that this appears to be intended
> to be used to store important state in pgstats. For which pgstats is
> fundamentally unsuitable (pgstat can loose state during normal operation,
> always looses state during crash restarts, the state can be reset).
The information on pg_stat_subscription_workers view, especially
last_error_xid, can be used to specify XID to "ALTER SUBSCRIPTION ...
SKIP (xid = XXX)" command which is proposed on the same thread, but it
doesn't mean that the new SKIP command relies on this information. The
failure XID is written in the server logs as well and the user
specifies XID manually.
>
> I don't really understand the name "pg_stat_subscription_workers" - what
> workers are stats kept about exactly? The columns don't obviously refer to a
> single worker or such? From the contents it should be name
> pg_stat_subscription_table_stats or such. But no, that'd not quite right,
> because apply errors are stored per-susbcription, while initial sync stuff is
> per-(subscription, table).
This stores stats for subscription workers namely apply and tablesync
worker, so named as pg_stat_subscription_workers.
Also, there is another proposal to add transaction statistics for
logical replication subscribers[1], and it's reasonable to merge these
statistics and this error information rather than having separate
views[2]. There also was an idea to add the transaction statistics to
pg_stat_subscription view, but it doesn't seem a good idea because the
pg_stat_subscription shows dynamic statistics whereas the transaction
statistics are accumulative statistics[3].
>
> The pgstat entries are quite wide (292 bytes), because of the error message
> stored. That's nearly twice the size of PgStat_StatTabEntry. And as far as I
> can tell, once there was an error, we'll just keep the stats entry around
> until the subscription is dropped.
We can drop the particular statistics by
pg_stat_reset_subscription_worker() function.
> And that includes stats for long dropped
> tables, as far as I can see - except that they're hidden from view, due to a
> join to pg_subscription_rel.
We are planning to drop this after successfully apply[4].
> To me this looks like it's using pgstat as an extremely poor IPC mechanism.
>
>
> Why isn't this just storing data in pg_subscription_rel?
These need to be updated on error which means for a failed xact and we
don't want to update the system catalog in that state. There will be
some challenges in a case where updating pg_subscription_rel also
failed too (what to report to the user, etc.). And moreover, we don't
want to consume space for temporary information in the system catalog.
Regards,
[1] https://www.postgresql.org/message-id/OSBPR01MB48887CA8F40C8D984A6DC00CED199%40OSBPR01MB4888.jpnprd0...
[2] https://www.postgresql.org/message-id/CAD21AoDF7LmSALzMfmPshRw_xFcRz3WvB-me8T2gO6Ht%3D3zL2w%40mail.g...
[3] https://www.postgresql.org/message-id/CAA4eK1JqwpsvjhLxV8CMYQ3NrhimZ8AFhWHh0Qn1FrL%3DLXfY6Q%40mail.g...
[4] https://www.postgresql.org/message-id/CAA4eK1%2B9yXkWkJSNtWYV2rG7QNAnoAt%2BeNH0PexoSP9ZQmXKPg%40mail...
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats
2022-01-25 06:31 Design of pg_stat_subscription_workers vs pgstats Andres Freund <[email protected]>
2022-01-25 11:27 ` Re: Design of pg_stat_subscription_workers vs pgstats Masahiko Sawada <[email protected]>
@ 2022-01-27 05:46 ` Andres Freund <[email protected]>
2022-02-15 18:31 ` Re: Design of pg_stat_subscription_workers vs pgstats Robert Haas <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Andres Freund @ 2022-01-27 05:46 UTC (permalink / raw)
To: Masahiko Sawada <[email protected]>; +Cc: pgsql-hackers; Amit Kapila <[email protected]>
Hi,
I didn't quite get to responding in depth, but I wanted to at least respond to
one point today.
On 2022-01-25 20:27:07 +0900, Masahiko Sawada wrote:
> > The pgstat entries are quite wide (292 bytes), because of the error message
> > stored. That's nearly twice the size of PgStat_StatTabEntry. And as far as I
> > can tell, once there was an error, we'll just keep the stats entry around
> > until the subscription is dropped.
>
> We can drop the particular statistics by
> pg_stat_reset_subscription_worker() function.
Only if either the user wants to drop all stats, or somehow knows the oids of
already dropped tables...
> > Why isn't this just storing data in pg_subscription_rel?
>
> These need to be updated on error which means for a failed xact and we
> don't want to update the system catalog in that state.
Rightly so! In fact, I'm concerned with sending a pgstats message in that
state as well. But: You don't need to. Just abort the current transaction,
start a new one, and update the state.
> There will be some challenges in a case where updating pg_subscription_rel
> also failed too (what to report to the user, etc.). And moreover, we don't
> want to consume space for temporary information in the system catalog.
You're consuming resources in a *WAY* worse way right now. The stats file gets
constantly written out, and quite often read back by backends. In contrast to
parts of pg_subscription_rel or such that data can't be removed from
shared_buffers under pressure.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats
2022-01-25 06:31 Design of pg_stat_subscription_workers vs pgstats Andres Freund <[email protected]>
2022-01-25 11:27 ` Re: Design of pg_stat_subscription_workers vs pgstats Masahiko Sawada <[email protected]>
2022-01-27 05:46 ` Re: Design of pg_stat_subscription_workers vs pgstats Andres Freund <[email protected]>
@ 2022-02-15 18:31 ` Robert Haas <[email protected]>
2022-02-16 08:50 ` Re: Design of pg_stat_subscription_workers vs pgstats Amit Kapila <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Robert Haas @ 2022-02-15 18:31 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Masahiko Sawada <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]>
On Thu, Jan 27, 2022 at 12:46 AM Andres Freund <[email protected]> wrote:
> Only if either the user wants to drop all stats, or somehow knows the oids of
> already dropped tables...
If it's really true that we can end up storing data for dropped
objects, I think that's not acceptable and needs to be fixed.
I don't currently understand the other issues on this thread well
enough to have a clear opinion on them.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Design of pg_stat_subscription_workers vs pgstats
2022-01-25 06:31 Design of pg_stat_subscription_workers vs pgstats Andres Freund <[email protected]>
2022-01-25 11:27 ` Re: Design of pg_stat_subscription_workers vs pgstats Masahiko Sawada <[email protected]>
2022-01-27 05:46 ` Re: Design of pg_stat_subscription_workers vs pgstats Andres Freund <[email protected]>
2022-02-15 18:31 ` Re: Design of pg_stat_subscription_workers vs pgstats Robert Haas <[email protected]>
@ 2022-02-16 08:50 ` Amit Kapila <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Amit Kapila @ 2022-02-16 08:50 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Masahiko Sawada <[email protected]>; pgsql-hackers
On Wed, Feb 16, 2022 at 12:01 AM Robert Haas <[email protected]> wrote:
>
> On Thu, Jan 27, 2022 at 12:46 AM Andres Freund <[email protected]> wrote:
> > Only if either the user wants to drop all stats, or somehow knows the oids of
> > already dropped tables...
>
> If it's really true that we can end up storing data for dropped
> objects, I think that's not acceptable and needs to be fixed.
>
Agreed.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2022-02-16 08:50 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-03-04 09:25 [PATCH 2/8] Expand the external data before forming the tuple Dilip Kumar <[email protected]>
2022-01-25 06:31 Design of pg_stat_subscription_workers vs pgstats Andres Freund <[email protected]>
2022-01-25 11:27 ` Re: Design of pg_stat_subscription_workers vs pgstats Masahiko Sawada <[email protected]>
2022-01-27 05:46 ` Re: Design of pg_stat_subscription_workers vs pgstats Andres Freund <[email protected]>
2022-02-15 18:31 ` Re: Design of pg_stat_subscription_workers vs pgstats Robert Haas <[email protected]>
2022-02-16 08:50 ` Re: Design of pg_stat_subscription_workers vs pgstats Amit Kapila <[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