Thread: Skip adding row-marks for non target tables when result relation is foreign table.
Skip adding row-marks for non target tables when result relation is foreign table.
From
SAIKIRAN AVULA
Date:
Hi PostgreSQL Community,
I would like to bring to your attention an observation regarding the planner's behavior for foreign table update/delete operations. It appears that the planner adds rowmarks (ROW_MARK_COPY) for non-target tables, which I believe is unnecessary when using the postgres-fdw. This is because postgres-fdw performs early locking on tuples belonging to the target foreign table by utilizing the SELECT FOR UPDATE clause.
In an attempt to address this, I tried implementing late locking. However, this approach still doesn't work as intended because the API assumes that foreign table rows can be re-fetched using TID (ctid). This assumption is invalid for partitioned tables on the foreign server. Additionally, the commit afb9249d06f47d7a6d4a89fea0c3625fe43c5a5d, which introduced late locking for foreign tables, mentions that the benefits of late locking against a remote server are unclear, as the extra round trips required are likely to outweigh any potential concurrency improvements.
To address this issue, I have taken the initiative to create a patch that prevents the addition of rowmarks for non-target tables when the target table is using early locking. I would greatly appreciate it if you could review the patch and provide any feedback or insights I may be overlooking.
Example query plan with my change: (foreign scan doesn't fetch whole row for bar).
postgres=# \d+ bar
Foreign table "public.bar"
Column | Type | Collation | Nullable | Default | FDW options | Storage | Stats target | Description
--------+---------+-----------+----------+---------+--------------------+---------+--------------+-------------
b1 | integer | | | | (column_name 'b1') | plain | |
b2 | integer | | | | (column_name 'b2') | plain | |
Server: postgres_1
FDW options: (schema_name 'public', table_name 'bar')
router=# \d+ foo
Foreign table "public.foo"
Column | Type | Collation | Nullable | Default | FDW options | Storage | Stats target | Description
--------+---------+-----------+----------+---------+--------------------+---------+--------------+-------------
f1 | integer | | | | (column_name 'f1') | plain | |
f2 | integer | | | | (column_name 'f2') | plain | |
Server: postgres_2
FDW options: (schema_name 'public', table_name 'foo')
postgres=# explain verbose update foo set f1 = b1 from bar where f2=b2;
QUERY PLAN
----------------------------------------------------------------------------------------
Update on public.foo (cost=200.00..48713.72 rows=0 width=0)
Remote SQL: UPDATE public.foo SET f1 = $2 WHERE ctid = $1
-> Nested Loop (cost=200.00..48713.72 rows=15885 width=42)
Output: bar.b1, foo.ctid, foo.*
Join Filter: (foo.f2 = bar.b2)
-> Foreign Scan on public.bar (cost=100.00..673.20 rows=2560 width=8)
Output: bar.b1, bar.b2
Remote SQL: SELECT b1, b2 FROM public.bar
-> Materialize (cost=100.00..389.23 rows=1241 width=42)
Output: foo.ctid, foo.*, foo.f2
-> Foreign Scan on public.foo (cost=100.00..383.02 rows=1241 width=42)
Output: foo.ctid, foo.*, foo.f2
Remote SQL: SELECT f1, f2, ctid FROM public.foo FOR UPDATE
(13 rows)
Foreign table "public.bar"
Column | Type | Collation | Nullable | Default | FDW options | Storage | Stats target | Description
--------+---------+-----------+----------+---------+--------------------+---------+--------------+-------------
b1 | integer | | | | (column_name 'b1') | plain | |
b2 | integer | | | | (column_name 'b2') | plain | |
Server: postgres_1
FDW options: (schema_name 'public', table_name 'bar')
router=# \d+ foo
Foreign table "public.foo"
Column | Type | Collation | Nullable | Default | FDW options | Storage | Stats target | Description
--------+---------+-----------+----------+---------+--------------------+---------+--------------+-------------
f1 | integer | | | | (column_name 'f1') | plain | |
f2 | integer | | | | (column_name 'f2') | plain | |
Server: postgres_2
FDW options: (schema_name 'public', table_name 'foo')
postgres=# explain verbose update foo set f1 = b1 from bar where f2=b2;
QUERY PLAN
----------------------------------------------------------------------------------------
Update on public.foo (cost=200.00..48713.72 rows=0 width=0)
Remote SQL: UPDATE public.foo SET f1 = $2 WHERE ctid = $1
-> Nested Loop (cost=200.00..48713.72 rows=15885 width=42)
Output: bar.b1, foo.ctid, foo.*
Join Filter: (foo.f2 = bar.b2)
-> Foreign Scan on public.bar (cost=100.00..673.20 rows=2560 width=8)
Output: bar.b1, bar.b2
Remote SQL: SELECT b1, b2 FROM public.bar
-> Materialize (cost=100.00..389.23 rows=1241 width=42)
Output: foo.ctid, foo.*, foo.f2
-> Foreign Scan on public.foo (cost=100.00..383.02 rows=1241 width=42)
Output: foo.ctid, foo.*, foo.f2
Remote SQL: SELECT f1, f2, ctid FROM public.foo FOR UPDATE
(13 rows)
Thank you for your time and consideration.
Regards
Saikiran Avula
SDE, Amazon Web Services.
Attachment
Re: Skip adding row-marks for non target tables when result relation is foreign table.
From
Jeff Davis
Date:
On Mon, 2024-05-06 at 23:10 +0100, SAIKIRAN AVULA wrote: > I would like to bring to your attention an observation regarding the > planner's behavior for foreign table update/delete operations. It > appears that the planner adds rowmarks (ROW_MARK_COPY) for non-target > tables, which I believe is unnecessary when using the postgres-fdw. > This is because postgres-fdw performs early locking on tuples > belonging to the target foreign table by utilizing the SELECT FOR > UPDATE clause. I agree with your reasoning here. If it reads the row with SELECT FOR UPDATE, what's the purpose of row marks? The cost of ROW_MARK_COPY is that it brings the whole tuple along rather than a reference. I assume you are concerned about wide tables involved in the join or is there another concern? > In an attempt to address this, I tried implementing late locking. For others in the thread, see: https://www.postgresql.org/docs/current/fdw-row-locking.html > However, this approach still doesn't work as intended because the API > assumes that foreign table rows can be re-fetched using TID (ctid). > This assumption is invalid for partitioned tables on the foreign > server. It looks like it's a "Datum rowid", but is currently only allowed to be a ctid, which can't identify the partition. I wonder how much work it would be to fix this? > Additionally, the commit afb9249d06f47d7a6d4a89fea0c3625fe43c5a5d, > which introduced late locking for foreign tables, mentions that the > benefits of late locking against a remote server are unclear, as the > extra round trips required are likely to outweigh any potential > concurrency improvements. The extra round trip only happens when EPQ finds a newer version of the tuple, which should be the exceptional case. I'm not sure how this balances out, but to me late locking still seems preferable. Early locking is a huge performance hit in some cases (locking many more rows than necessary). Early locking is also a violation of the documentation here: "When a locking clause appears at the top level of a SELECT query, the rows that are locked are exactly those that are returned by the query; in the case of a join query, the rows locked are those that contribute to returned join rows." https://www.postgresql.org/docs/current/sql-select.html#SQL-FOR-UPDATE-SHARE > To address this issue, I have taken the initiative to create a patch > that prevents the addition of rowmarks for non-target tables when the > target table is using early locking. I would greatly appreciate it if > you could review the patch and provide any feedback or insights I may > be overlooking. A couple comments: * You're using GetFdwRoutineForRelation() with makecopy=false, and then closing the relation. If the rd_fdwroutine was already set previously, then the returned pointer will point into the relcache, which may be invalid after closing the relation. I'd probably pass makecopy=true and then free it. (Weirdly if you pass makecopy=false, you may or may not get a copy, so there's no way to know whether to free it or not.) * Core postgres doesn't really choose early locking. If RefetchForeignRow is not defined, then late locking is impossible, so it assumes that early locking is happening. That assumption is true for postgres_fdw, but might not be for other FDWs. What if an FDW doesn't do early locking and also doesn't define RefetchForeignRow? Regards, Jeff Davis
Re: Skip adding row-marks for non target tables when result relation is foreign table.
From
Etsuro Fujita
Date:
On Wed, May 22, 2024 at 10:13 AM Jeff Davis <pgsql@j-davis.com> wrote: > On Mon, 2024-05-06 at 23:10 +0100, SAIKIRAN AVULA wrote: > > Additionally, the commit afb9249d06f47d7a6d4a89fea0c3625fe43c5a5d, > > which introduced late locking for foreign tables, mentions that the > > benefits of late locking against a remote server are unclear, as the > > extra round trips required are likely to outweigh any potential > > concurrency improvements. > > The extra round trip only happens when EPQ finds a newer version of the > tuple, which should be the exceptional case. I'm not sure how this > balances out, but to me late locking still seems preferable. Early > locking is a huge performance hit in some cases (locking many more rows > than necessary). I might be missing something, but I think the extra round trip happens for each foreign row locked using the RefetchForeignRow() API in ExecLockRows(), so I think the overhead is caused in the normal case. > Early locking is also a violation of the documentation here: > > "When a locking clause appears at the top level of a SELECT query, the > rows that are locked are exactly those that are returned by the query; > in the case of a join query, the rows locked are those that contribute > to returned join rows." Yeah, but I think this holds true for SELECT queries postgres_fdw sends to the remote side. :) Best regards, Etsuro Fujita
Re: Skip adding row-marks for non target tables when result relation is foreign table.
From
Jeff Davis
Date:
On Fri, 2024-08-09 at 17:35 +0900, Etsuro Fujita wrote:
> I might be missing something, but I think the extra round trip
> happens
> for each foreign row locked using the RefetchForeignRow() API in
> ExecLockRows(), so I think the overhead is caused in the normal case.
Is there any sample code that implements late locking for a FDW? I'm
not quite clear on how it's supposed to work.
Regards,
Jeff Davis
Re: Skip adding row-marks for non target tables when result relation is foreign table.
From
Etsuro Fujita
Date:
On Thu, Aug 15, 2024 at 9:56 AM Jeff Davis <pgsql@j-davis.com> wrote: > Is there any sample code that implements late locking for a FDW? I'm > not quite clear on how it's supposed to work. See the patch in [1]. It would not apply to HEAD anymore, though. Best regards, Etsuro Fujita [1] https://www.postgresql.org/message-id/16016.1431455074@sss.pgh.pa.us