From: | Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Jim Nasby <Jim(dot)Nasby(at)BlueTreble(dot)com>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, ashutosh(dot)bapat(at)enterprisedb(dot)com, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: EvalPlanQual behaves oddly for FDW queries involving system columns |
Date: | 2015-05-11 13:00:55 |
Message-ID: | 5550A807.6040700@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2015/05/11 8:50, Tom Lane wrote:
> Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> writes:
>> [ EvalPlanQual-v6.patch ]
>
> I've started to study this in a little more detail, and I'm not terribly
> happy with some of the API decisions in it.
Thanks for taking the time to review the patch!
> In particular, I find the addition of "void *fdw_state" to ExecRowMark
> to be pretty questionable. That does not seem like a good place to keep
> random state. (I realize that WHERE CURRENT OF keeps some state in
> ExecRowMark, but that's a crock not something to emulate.) ISTM that in
> most scenarios, the state that LockForeignRow/FetchForeignRow would like
> to get at is probably the FDW state associated with the ForeignScanState
> that the tuple came from. Which this API doesn't help with particularly.
> I wonder if we should instead add a "ScanState*" field and expect the
> core code to set that up (ExecOpenScanRelation could do it with minor
> API changes...).
Sorry, I don't understand clearly what you mean, but that (the idea of
expecting the core to set it up) sounds inconsistent with your comment
on the earlier version of the API "BeginForeignFetch" [1].
> I'm also a bit tempted to pass the TIDs to LockForeignRow and
> FetchForeignRow as Datums not ItemPointers. We have the Datum format
> available already at the call sites, so this is free as far as the core
> code is concerned, and would only cost another line or so for the FDWs.
> This is by no means sufficient to allow FDWs to use some other type than
> "tid" for row identifiers; but it would be a down payment on that problem,
> and at least would avoid nailing the rowids-are-tids assumption into yet
> another global API.
That is a good idea.
> Also, as I mentioned, I'd be a whole lot happier if we had a way to test
> this...
Attached is a postgres_fdw patch that I used for the testing. If you
try it, edit postgresGetForeignRowMarkType as necessary. I have to
confess that I did the testing only in the normal conditions by the patch.
Sorry for the delay. I took a vacation until yesterday.
Best regards,
Etsuro Fujita
[1] http://www.postgresql.org/message-id/14504.1428446647@sss.pgh.pa.us
Attachment | Content-Type | Size |
---|---|---|
EvalPlanQual-postgres_fdw-test.patch | text/x-diff | 27.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Stephen Frost | 2015-05-11 13:32:46 | Re: LOCK TABLE Permissions |
Previous Message | Stephen Frost | 2015-05-11 12:32:02 | Re: "unaddressable bytes" in BRIN |