pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedLogical replication row filtering and TOAST
6+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2022-04-06 2d09e44d30 Improve comments for row filtering and toast interaction in logical replication.
* Logical replication row filtering and TOAST
@ 2022-04-05 09:50 Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 6+ messages in thread
From: Antonin Houska @ 2022-04-05 09:50 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
I spent some time thinking about a special case of evaluation of the row
filter and wrote a comment that might be useful (see the attachment). However
now I think that it's not perfect if the code really relies on the fact that
value of an indexed column cannot be TOASTed due to size restrictions.
I could hit two different error messages when trying activate TOAST on an
index column (in this case PG was build with 16kB pages), but still I think
the code is unnecessarily fragile if it relies on such errors:
ERROR: index row requires 8224 bytes, maximum size is 8191
ERROR: index row size 8048 exceeds btree version 4 maximum 5432 for index "b_pkey"
DETAIL: Index row references tuple (0,3) in relation "b".
HINT: Values larger than 1/3 of a buffer page cannot be indexed.
Note that at least in ExtractReplicaIdentity() we do expect that an indexed
column value can be TOASTed.
/*
* If the tuple, which by here only contains indexed columns, still has
* toasted columns, force them to be inlined. This is somewhat unlikely
* since there's limits on the size of indexed columns, so we don't
* duplicate toast_flatten_tuple()s functionality in the above loop over
* the indexed columns, even if it would be more efficient.
*/
if (HeapTupleHasExternal(key_tuple))
{
HeapTuple oldtup = key_tuple;
key_tuple = toast_flatten_tuple(oldtup, desc);
heap_freetuple(oldtup);
}
Do I miss anything?
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
Attachments:
logical_rep_row_filter_comment.diff (text/x-diff, 912B) download
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 20d0b1e1253..2be5aaa18c4 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -1246,6 +1246,15 @@ pgoutput_row_filter(Relation relation, TupleTableSlot *old_slot,
*/
if (!new_slot || !old_slot)
{
+ /*
+ * For UPDATE, we should only get here if the replica identity is !=
+ * FULL and thus all the identity columns are index columns, i.e.
+ * never TOASTed. Therefore, we don't need to care of the unchanged
+ * toasted replica identity columns like we do below.
+ */
+ Assert(relation->rd_rel->relreplident != REPLICA_IDENTITY_FULL ||
+ map_changetype_pubaction[*action] != PUBACTION_UPDATE);
+
ecxt->ecxt_scantuple = new_slot ? new_slot : old_slot;
result = pgoutput_row_filter_exec_expr(filter_exprstate, ecxt);
* Re: Logical replication row filtering and TOAST
@ 2022-04-05 10:22 Antonin Houska <ah@cybertec.at>
parent: Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 6+ messages in thread
From: Antonin Houska @ 2022-04-05 10:22 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
Antonin Houska <ah@cybertec.at> wrote:
> I spent some time thinking about a special case of evaluation of the row
> filter and wrote a comment that might be useful (see the attachment). However
> now I think that it's not perfect if the code really relies on the fact that
> value of an indexed column cannot be TOASTed due to size restrictions.
>
> I could hit two different error messages when trying activate TOAST on an
> index column (in this case PG was build with 16kB pages), but still I think
> the code is unnecessarily fragile if it relies on such errors:
>
>
> ERROR: index row requires 8224 bytes, maximum size is 8191
>
> ERROR: index row size 8048 exceeds btree version 4 maximum 5432 for index "b_pkey"
> DETAIL: Index row references tuple (0,3) in relation "b".
> HINT: Values larger than 1/3 of a buffer page cannot be indexed.
>
>
> Note that at least in ExtractReplicaIdentity() we do expect that an indexed
> column value can be TOASTed.
>
> /*
> * If the tuple, which by here only contains indexed columns, still has
> * toasted columns, force them to be inlined. This is somewhat unlikely
> * since there's limits on the size of indexed columns, so we don't
> * duplicate toast_flatten_tuple()s functionality in the above loop over
> * the indexed columns, even if it would be more efficient.
> */
> if (HeapTupleHasExternal(key_tuple))
> {
> HeapTuple oldtup = key_tuple;
>
> key_tuple = toast_flatten_tuple(oldtup, desc);
> heap_freetuple(oldtup);
> }
>
> Do I miss anything?
Well, I see now that the point might be that, in heap_update(),
"id_has_external" would be true the indexed value could be TOASTed, so that
the (flattened) old tuple would be WAL logged:
old_key_tuple = ExtractReplicaIdentity(relation, &oldtup,
bms_overlap(modified_attrs, id_attrs) ||
id_has_external,
&old_key_copied);
Nevertheless, a comment in pgoutput_row_filter(), saying that TOASTed values
are not expected if old_slot is NULL, might be useful.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Logical replication row filtering and TOAST
@ 2022-04-05 10:32 Amit Kapila <amit.kapila16@gmail.com>
parent: Antonin Houska <ah@cybertec.at>
0 siblings, 2 replies; 6+ messages in thread
From: Amit Kapila @ 2022-04-05 10:32 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Apr 5, 2022 at 3:52 PM Antonin Houska <ah@cybertec.at> wrote:
>
> Antonin Houska <ah@cybertec.at> wrote:
>
> > I spent some time thinking about a special case of evaluation of the row
> > filter and wrote a comment that might be useful (see the attachment). However
> > now I think that it's not perfect if the code really relies on the fact that
> > value of an indexed column cannot be TOASTed due to size restrictions.
> >
> > I could hit two different error messages when trying activate TOAST on an
> > index column (in this case PG was build with 16kB pages), but still I think
> > the code is unnecessarily fragile if it relies on such errors:
> >
> >
> > ERROR: index row requires 8224 bytes, maximum size is 8191
> >
> > ERROR: index row size 8048 exceeds btree version 4 maximum 5432 for index "b_pkey"
> > DETAIL: Index row references tuple (0,3) in relation "b".
> > HINT: Values larger than 1/3 of a buffer page cannot be indexed.
> >
> >
> > Note that at least in ExtractReplicaIdentity() we do expect that an indexed
> > column value can be TOASTed.
> >
> > /*
> > * If the tuple, which by here only contains indexed columns, still has
> > * toasted columns, force them to be inlined. This is somewhat unlikely
> > * since there's limits on the size of indexed columns, so we don't
> > * duplicate toast_flatten_tuple()s functionality in the above loop over
> > * the indexed columns, even if it would be more efficient.
> > */
> > if (HeapTupleHasExternal(key_tuple))
> > {
> > HeapTuple oldtup = key_tuple;
> >
> > key_tuple = toast_flatten_tuple(oldtup, desc);
> > heap_freetuple(oldtup);
> > }
> >
> > Do I miss anything?
>
> Well, I see now that the point might be that, in heap_update(),
> "id_has_external" would be true the indexed value could be TOASTed, so that
> the (flattened) old tuple would be WAL logged:
>
Right.
>
> Nevertheless, a comment in pgoutput_row_filter(), saying that TOASTed values
> are not expected if old_slot is NULL, might be useful.
>
How about something like the attached?
--
With Regards,
Amit Kapila.
Attachments:
update_toast_comment_1.patch (application/octet-stream, 1.1K) download
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 20d0b1e125..9d33630464 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -1237,10 +1237,11 @@ pgoutput_row_filter(Relation relation, TupleTableSlot *old_slot,
* For inserts, we only have the new tuple.
*
* For updates, we can have only a new tuple when none of the replica
- * identity columns changed but we still need to evaluate the row filter
- * for new tuple as the existing values of those columns might not match
- * the filter. Also, users can use constant expressions in the row filter,
- * so we anyway need to evaluate it for the new tuple.
+ * identity columns changed and none of those columns have external data
+ * but we still need to evaluate the row filter for the new tuple as the
+ * existing values of those columns might not match the filter. Also, users
+ * can use constant expressions in the row filter, so we anyway need to
+ * evaluate it for the new tuple.
*
* For deletes, we only have the old tuple.
*/
* Re: Logical replication row filtering and TOAST
@ 2022-04-05 11:59 Antonin Houska <ah@cybertec.at>
parent: Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 0 replies; 6+ messages in thread
From: Antonin Houska @ 2022-04-05 11:59 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Amit Kapila <amit.kapila16@gmail.com> wrote:
> > Antonin Houska <ah@cybertec.at> wrote:
> >
> > Nevertheless, a comment in pgoutput_row_filter(), saying that TOASTed values
> > are not expected if old_slot is NULL, might be useful.
> >
>
> How about something like the attached?
Yes, that'd be sufficient. Thanks.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Logical replication row filtering and TOAST
@ 2022-04-06 01:50 Ajin Cherian <itsajin@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 1 reply; 6+ messages in thread
From: Ajin Cherian @ 2022-04-06 01:50 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Antonin Houska <ah@cybertec.at>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Apr 5, 2022 at 8:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> How about something like the attached?
>
LGTM.
regards,
Ajin Cherian
Fujitsu Australia
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Logical replication row filtering and TOAST
@ 2022-04-06 04:33 Amit Kapila <amit.kapila16@gmail.com>
parent: Ajin Cherian <itsajin@gmail.com>
0 siblings, 0 replies; 6+ messages in thread
From: Amit Kapila @ 2022-04-06 04:33 UTC (permalink / raw)
To: Ajin Cherian <itsajin@gmail.com>; +Cc: Antonin Houska <ah@cybertec.at>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, Apr 6, 2022 at 7:21 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Tue, Apr 5, 2022 at 8:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> >
> > How about something like the attached?
> >
>
> LGTM.
>
Pushed.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2022-04-06 04:33 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-04-05 09:50 Logical replication row filtering and TOAST Antonin Houska <ah@cybertec.at>
2022-04-05 10:22 ` Antonin Houska <ah@cybertec.at>
2022-04-05 10:32 ` Amit Kapila <amit.kapila16@gmail.com>
2022-04-05 11:59 ` Antonin Houska <ah@cybertec.at>
2022-04-06 01:50 ` Ajin Cherian <itsajin@gmail.com>
2022-04-06 04:33 ` Amit Kapila <amit.kapila16@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox