Thread: Forbid to DROP temp tables of other sessions
Hi,
I noticed that TRUNCATE and ALTER commands on temporary tables of other sessions produce an error "cannot truncate/alter temporary tables of other sessions". But are there any reasons to allow us to DROP such tables?
It seems to me that the only case when we may need it is the removal of orphan tables. But the autovacuum is responsible for this and it uses a different functionality. I'm wondering if there are any other cases. If not, can we just handle it for example in ExecDropStmt and produce an error like "cannot drop temporary tables of other sessions"?
--
Best regards,
Daniil Davydov
On Fri, 25 Oct 2024 at 11:02, Daniil Davydov <3danissimo@gmail.com> wrote:
But are there any reasons to allow us to DROP such tables?
Hi!
This topic has already been discussed in [0], I believe. I'm not sure how it all ended and if there were any changes made in the end. But from the user's perspective, temporary tables are expected to be isolated within sessions, I think. This is an ideal solution, but does it feasible or not is a question.
BTW, if we can "isolate" temp relations, we'll be one step close to get rid of temp relations locking [1].
[0] https://www.postgresql.org/message-id/flat/d4a68c6d-d6c4-d52a-56cb-babb8177b5fe%40oss.nttdata.com
Best regards,
Maxim Orlov.
Daniil Davydov <3danissimo@gmail.com> writes:
> I noticed that TRUNCATE and ALTER commands on temporary tables of other
> sessions produce an error "cannot truncate/alter temporary tables of other
> sessions". But are there any reasons to allow us to DROP such tables?
> It seems to me that the only case when we may need it is the removal of
> orphan tables. But the autovacuum is responsible for this and it uses a
> different functionality. I'm wondering if there are any other cases. If
> not, can we just handle it for example in ExecDropStmt and produce an error
> like "cannot drop temporary tables of other sessions"?
If autovacuum can do it, I don't see a reason to prevent superusers
from doing it manually.
regards, tom lane
Hi, looks good for me, but please fix formatting in temp_tbl_fix.patch!
On Tue, 29 Oct 2024 at 07:22, Daniil Davydov <3danissimo@gmail.com> wrote:
Hi,
Thanks for your comments, I appreciate them.
As I continued to deal with the topic of working with temp tables of
other sessions, I noticed something like a bug. For example
(REL_17_STABLE):
Session 1:
=# CREATE TEMP TABLE test(id int);
Session 2:
=# INSERT INTO pg_temp_0.test VALUES (1);
=# INSERT INTO pg_temp_0.test VALUES (2);
Second INSERT command will end with an error "cannot access temporary
tables of other sessions". I checked why this is happening and found
errors in several places.
Good catch. I agree with this being an unwarranted behaviour.
A minor comment from my end is the wording of the error message.
Based on the Postgresql error message style huide, something like this could be better,
"could not access temporary relations of other sessions".
So, I attach two files to this email :
1) Isolation test, that shows an error in REL_17_STABLE (iso_1.patch)
2) Patch that fixes code that mistakenly considered temporary tables
to be permanent (I will be glad to receive feedback on these fixes) +
isolation test, which shows that now any action with temp table of
other session leads to error (temp_tbl_fix.patch)
Tests look kinda ugly, but I think it's inevitable, given that we
don't know exactly what the name of the temporary schema of other
session will be.
--
Best regards,
Daniil Davydov
Regards,
Rafia Sabih
Rafia Sabih
CYBERTEC PostgreSQL International GmbH
On Thu, 14 Nov 2024 at 09:55, Daniil Davydov <3danissimo@gmail.com> wrote:
On Wed, Oct 30, 2024 at 7:32 PM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
> Good catch. I agree with this being an unwarranted behaviour.
> A minor comment from my end is the wording of the error message.
> Based on the Postgresql error message style huide, something like this could be better,
> "could not access temporary relations of other sessions".
> --
> Regards,
> Rafia Sabih
> CYBERTEC PostgreSQL International GmbH
>
Thanks for your comment. I attach a patch with a fixed error message.
Also you can find it in commit fest
(https://commitfest.postgresql.org/51/5379/)
Looks good to me.
--
Best Regards,
Daniil Davydov
Regards,
Rafia Sabih
Rafia Sabih
CYBERTEC PostgreSQL International GmbH
> On 14 Nov 2024, at 11:55, Daniil Davydov <3danissimo@gmail.com> wrote: > > On Wed, Oct 30, 2024 at 7:32 PM Rafia Sabih <rafia.pghackers@gmail.com> wrote: > >> Good catch. I agree with this being an unwarranted behaviour. >> A minor comment from my end is the wording of the error message. >> Based on the Postgresql error message style huide, something like this could be better, >> "could not access temporary relations of other sessions". >> -- >> Regards, >> Rafia Sabih >> CYBERTEC PostgreSQL International GmbH >> > Thanks for your comment. I attach a patch with a fixed error message. > Also you can find it in commit fest > (https://commitfest.postgresql.org/51/5379/) I suspect that protection of temp tables was broken by 00d1e02be249. And I'd suggest fixing it in a line with how it workedbefore. Changes to locking mechanism is kind of a super subtle matters, it is really hard to bring this checks herewithout breaking something else. Maybe not immidiately. but still. I'd suggest fixing somewhere around RelationAddBlocks().But be sure to check all code pathes that lead to this check. Also, having an isolation test is nice. But do we actually do isolation tests with PL\pgSQL? Thanks! Best regards, Andrey Borodin.
Hi, On 2024-11-21 23:52:52 +0300, Andrey M. Borodin wrote: > I suspect that protection of temp tables was broken by 00d1e02be249. I think we might have broken this in multiple ways in recent releases. In 17 one can even read the data from the other relation when using a sequential scan, because that'll go through a streaming read and from there directly to StartReadBuffers(), bypassing the check in ReadBufferExtended(). > And I'd suggest fixing it in a line with how it worked before. Changes to > locking mechanism is kind of a super subtle matters, it is really hard to > bring this checks here without breaking something else. Maybe not > immidiately. but still. I'd suggest fixing somewhere around > RelationAddBlocks(). But be sure to check all code pathes that lead to this > check. Yea, I don't think the lock approach would work that well. However, I don't love having to put RELATION_IS_OTHER_TEMP() checks everywhere either. After all we seem to have introduced two independent oversights related to this... I wonder if we could handle this by having a few locations explicitly opt-in to accessing another database's temp table and erroring out everywhere else - there's not that many places we need to do so. > Also, having an isolation test is nice. But do we actually do isolation > tests with PL\pgSQL? There are several other tests creating functions. But I think this one goes a bit too far... Greetings, Andres Freund