Use RELATION_IS_OTHER_TEMP where possible

Started by Junwang Zhao7 months ago8 messages
#1Junwang Zhao
zhjwpku@gmail.com

Hi hackers,

While reviewing the Merge/Split partitions patch[0]/messages/by-id/884bcf9e-d6e3-4b0c-8dcb-7f2110070ac9@postgrespro.ru, I found
some places in tablecmds.c where RELATION_IS_OTHER_TEMP
can be used but not, such as:

/* If the parent is temp, it must belong to this session */
if (rel->rd_rel->relpersistence == RELPERSISTENCE_TEMP &&
!rel->rd_islocaltemp)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot attach as partition of temporary relation of another session")));

/* Ditto for the partition */
if (attachrel->rd_rel->relpersistence == RELPERSISTENCE_TEMP &&
!attachrel->rd_islocaltemp)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot attach temporary relation of another session as partition")));

All other files perform this check using RELATION_IS_OTHER_TEMP.
Should we update tablecmds.c to do the same for consistency?

[0]: /messages/by-id/884bcf9e-d6e3-4b0c-8dcb-7f2110070ac9@postgrespro.ru

--
Regards
Junwang Zhao

#2Nathan Bossart
nathandbossart@gmail.com
In reply to: Junwang Zhao (#1)
Re: Use RELATION_IS_OTHER_TEMP where possible

On Wed, Jun 11, 2025 at 12:07:35AM +0800, Junwang Zhao wrote:

All other files perform this check using RELATION_IS_OTHER_TEMP.
Should we update tablecmds.c to do the same for consistency?

Seems like a good idea.

--
nathan

#3Junwang Zhao
zhjwpku@gmail.com
In reply to: Nathan Bossart (#2)
1 attachment(s)
Re: Use RELATION_IS_OTHER_TEMP where possible

Hi Nathan,

On Wed, Jun 11, 2025 at 12:30 AM Nathan Bossart
<nathandbossart@gmail.com> wrote:

On Wed, Jun 11, 2025 at 12:07:35AM +0800, Junwang Zhao wrote:

All other files perform this check using RELATION_IS_OTHER_TEMP.
Should we update tablecmds.c to do the same for consistency?

Seems like a good idea.

Thanks for the comment.

I have attached a patch with the proposed changes.

--
nathan

--
Regards
Junwang Zhao

Attachments:

v1-0001-use-RELATION_IS_OTHER_TEMP-wherever-possibile.patchapplication/octet-stream; name=v1-0001-use-RELATION_IS_OTHER_TEMP-wherever-possibile.patchDownload
From 2a484012cdcda3be1fec6dbbcfee9965a3fbdc16 Mon Sep 17 00:00:00 2001
From: Junwang Zhao <zhjwpku@gmail.com>
Date: Tue, 10 Jun 2025 23:33:17 +0000
Subject: [PATCH v1] use RELATION_IS_OTHER_TEMP wherever possibile

In some parts of tablecmds.c, the temporary session check is
performed without using RELATION_IS_OTHER_TEMP. Update tablecmds.c
to use RELATION_IS_OTHER_TEMP for this check.
---
 src/backend/commands/tablecmds.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ea96947d813..c17e3c42604 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2711,8 +2711,7 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 							RelationGetRelationName(relation))));
 
 		/* If existing rel is temp, it must belong to this session */
-		if (relation->rd_rel->relpersistence == RELPERSISTENCE_TEMP &&
-			!relation->rd_islocaltemp)
+		if (RELATION_IS_OTHER_TEMP(relation))
 			ereport(ERROR,
 					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 					 errmsg(!is_partition
@@ -17225,15 +17224,13 @@ ATExecAddInherit(Relation child_rel, RangeVar *parent, LOCKMODE lockmode)
 						RelationGetRelationName(parent_rel))));
 
 	/* If parent rel is temp, it must belong to this session */
-	if (parent_rel->rd_rel->relpersistence == RELPERSISTENCE_TEMP &&
-		!parent_rel->rd_islocaltemp)
+	if (RELATION_IS_OTHER_TEMP(parent_rel))
 		ereport(ERROR,
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 				 errmsg("cannot inherit from temporary relation of another session")));
 
 	/* Ditto for the child */
-	if (child_rel->rd_rel->relpersistence == RELPERSISTENCE_TEMP &&
-		!child_rel->rd_islocaltemp)
+	if (RELATION_IS_OTHER_TEMP(child_rel))
 		ereport(ERROR,
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 				 errmsg("cannot inherit to temporary relation of another session")));
@@ -20304,15 +20301,13 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd,
 						RelationGetRelationName(rel))));
 
 	/* If the parent is temp, it must belong to this session */
-	if (rel->rd_rel->relpersistence == RELPERSISTENCE_TEMP &&
-		!rel->rd_islocaltemp)
+	if (RELATION_IS_OTHER_TEMP(rel))
 		ereport(ERROR,
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 				 errmsg("cannot attach as partition of temporary relation of another session")));
 
 	/* Ditto for the partition */
-	if (attachrel->rd_rel->relpersistence == RELPERSISTENCE_TEMP &&
-		!attachrel->rd_islocaltemp)
+	if (RELATION_IS_OTHER_TEMP(attachrel))
 		ereport(ERROR,
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 				 errmsg("cannot attach temporary relation of another session as partition")));
-- 
2.39.5

#4Ashutosh Bapat
ashutosh.bapat.oss@gmail.com
In reply to: Junwang Zhao (#3)
Re: Use RELATION_IS_OTHER_TEMP where possible

On Wed, Jun 11, 2025 at 5:12 AM Junwang Zhao <zhjwpku@gmail.com> wrote:

Hi Nathan,

On Wed, Jun 11, 2025 at 12:30 AM Nathan Bossart
<nathandbossart@gmail.com> wrote:

On Wed, Jun 11, 2025 at 12:07:35AM +0800, Junwang Zhao wrote:

All other files perform this check using RELATION_IS_OTHER_TEMP.
Should we update tablecmds.c to do the same for consistency?

Seems like a good idea.

Thanks for the comment.

I have attached a patch with the proposed changes.

LGTM. I looked at other instances of is_localtemp, but none of them
have (relation)->rd_rel->relpersistence == RELPERSISTENCE_TEMP there. Your
patch has covered all the existing ones.

--
Best Wishes,
Ashutosh Bapat

#5Junwang Zhao
zhjwpku@gmail.com
In reply to: Ashutosh Bapat (#4)
Re: Use RELATION_IS_OTHER_TEMP where possible

Hi Ashutosh,

On Wed, Jun 11, 2025 at 7:11 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Wed, Jun 11, 2025 at 5:12 AM Junwang Zhao <zhjwpku@gmail.com> wrote:

Hi Nathan,

On Wed, Jun 11, 2025 at 12:30 AM Nathan Bossart
<nathandbossart@gmail.com> wrote:

On Wed, Jun 11, 2025 at 12:07:35AM +0800, Junwang Zhao wrote:

All other files perform this check using RELATION_IS_OTHER_TEMP.
Should we update tablecmds.c to do the same for consistency?

Seems like a good idea.

Thanks for the comment.

I have attached a patch with the proposed changes.

LGTM. I looked at other instances of is_localtemp, but none of them have (relation)->rd_rel->relpersistence == RELPERSISTENCE_TEMP there. Your patch has covered all the existing ones.

Thank you for the review and the additional check.

I have created a cf entry to track this.

https://commitfest.postgresql.org/patch/5815/

--
Best Wishes,
Ashutosh Bapat

--
Regards
Junwang Zhao

#6Nathan Bossart
nathandbossart@gmail.com
In reply to: Junwang Zhao (#5)
Re: Use RELATION_IS_OTHER_TEMP where possible

On Wed, Jun 11, 2025 at 07:35:39PM +0800, Junwang Zhao wrote:

I have created a cf entry to track this.

https://commitfest.postgresql.org/patch/5815/

I'll plan on committing this once v19 development begins.

--
nathan

#7Junwang Zhao
zhjwpku@gmail.com
In reply to: Nathan Bossart (#6)
Re: Use RELATION_IS_OTHER_TEMP where possible

On Fri, Jun 13, 2025 at 3:53 AM Nathan Bossart <nathandbossart@gmail.com> wrote:

On Wed, Jun 11, 2025 at 07:35:39PM +0800, Junwang Zhao wrote:

I have created a cf entry to track this.

https://commitfest.postgresql.org/patch/5815/

I'll plan on committing this once v19 development begins.

Sure, I just checked the CF entry, thanks for taking care of it.

--
nathan

--
Regards
Junwang Zhao

#8Nathan Bossart
nathandbossart@gmail.com
In reply to: Junwang Zhao (#7)
Re: Use RELATION_IS_OTHER_TEMP where possible

Committed.

--
nathan