WHERE CURRENT OF with RLS quals that are ctid conditions

On Mon, May 6, 2024 at 7:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert pointed out [1] that the planner fails if we have $SUBJECT,
because tidpath.c can seize on the RLS-derived ctid constraint
instead of the CurrentOfExpr. Since the executor can only handle
CurrentOfExpr in a TidScan's tidquals, that leads to a confusing
runtime error.

Here's a patch for that.

However ... along the way to testing it, I found that you can only
get such an RLS qual to work if it accepts "(InvalidBlockNumber,0)",
because that's what the ctid field will look like in a
not-yet-stored-to-disk tuple. That's sufficiently weird, and so
unduly in bed with undocumented implementation details, that I can't
imagine anyone is actually using such an RLS condition or ever will.
So maybe this is not really worth fixing. Thoughts?

Hmm, I thought the RLS condition needed to accept the old and new
TIDs, but not (InvalidBlockNumber,0). I might well have misunderstood,
though.

As to whether this is worth fixing, I think it is, but it might not be
worth back-patching the fix. Also, I'd really like to get disable_cost
out of the picture here. That would require more code reorganization
than you've done here, but I think it would be worthwhile. I suppose
that could also be done as a separate patch, but I wonder if that
doesn't just amount to changing approximately the same code twice.

Or maybe it doesn't, and this is worth doing on its own. I'm not sure;
I haven't coded what I have in mind yet.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, May 7, 2024 at 9:47 AM Robert Haas <robertmhaas@gmail.com> wrote:

As to whether this is worth fixing, I think it is, but it might not be
worth back-patching the fix. Also, I'd really like to get disable_cost
out of the picture here. That would require more code reorganization
than you've done here, but I think it would be worthwhile. I suppose
that could also be done as a separate patch, but I wonder if that
doesn't just amount to changing approximately the same code twice.

Or maybe it doesn't, and this is worth doing on its own. I'm not sure;
I haven't coded what I have in mind yet.

Never mind all this. I think what I have in mind requires doing what
you did first. So if you're happy with what you've got, I'd go for it.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, May 6, 2024 at 7:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

So maybe this is not really worth fixing. Thoughts?

Hmm, I thought the RLS condition needed to accept the old and new
TIDs, but not (InvalidBlockNumber,0). I might well have misunderstood,
though.

If you leave the (InvalidBlockNumber,0) alternative out of the RLS
condition, my patch's test case fails because the row "doesn't
satisfy the RLS condition" (I forget the exact error message, but
it was more or less that).

As to whether this is worth fixing, I think it is, but it might not be
worth back-patching the fix. Also, I'd really like to get disable_cost
out of the picture here. That would require more code reorganization
than you've done here, but I think it would be worthwhile. I suppose
that could also be done as a separate patch, but I wonder if that
doesn't just amount to changing approximately the same code twice.

No, because the disable_cost stuff is nowhere near here. In any case,
what we were talking about was suppressing creation of competing
non-TIDScan paths. It's still going to be incumbent on tidpath.c to
create a correct path, and as things stand it won't for this case.

regards, tom lane

Robert Haas <robertmhaas@gmail.com> writes:

Never mind all this. I think what I have in mind requires doing what
you did first. So if you're happy with what you've got, I'd go for it.

OK. HEAD-only sounds like a good compromise. Barring objections,
I'll do that later today.

regards, tom lane