Message-ID: <b8dbfbb9bc98e76f1424f6c64e79e122b88fc09d.camel@j-davis.com>
Subject: Re: Skip adding row-marks for non target tables when result
 relation is foreign table.
From: Jeff Davis <pgsql@j-davis.com>
To: SAIKIRAN AVULA <avulasaikiranreddy@gmail.com>, 
	pgsql-hackers@postgresql.org
Date: Tue, 21 May 2024 18:12:49 -0700
In-Reply-To: <CABg8Gm5z53jv7+2-=RXZ0XYOna7Ts_Yc+TP83qbBU4YDtbAouA@mail.gmail.com>
References: 
	<CABg8Gm5z53jv7+2-=RXZ0XYOna7Ts_Yc+TP83qbBU4YDtbAouA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/b8dbfbb9bc98e76f1424f6c64e79e122b88fc09d.camel%40j-davis.com>
Precedence: bulk

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-SHAR=
E

> 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=3Dfalse, 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=3Dtrue and
then free it. (Weirdly if you pass makecopy=3Dfalse, 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