Subject: Re: Partitioned tables and [un]loggedness

From: Shinya Kato <[email-masked]>
Date: Wed, 4 Jun 2025 18:55:15 +0900

On Wed, Jun 4, 2025 at 6:42 PM Michael Paquier <[email-masked]> wrote:
>
> On Thu, Sep 19, 2024 at 01:08:33PM +0900, Michael Paquier wrote:
> > I have applied 0001 for now to add ATT_PARTITIONED_TABLE.  Attached is
> > the remaining piece.
>
> And the second piece is now applied as of e2bab2d79204.
> --
> Michael

Hi,

Should we consider preventing tab completion for PARTITION BY
immediately after CREATE TABLE name (...)? Or is it fine to leave it
as is, given that it's syntactically correct?

-- 
Best regards,
Shinya Kato
NTT OSS Center

---

From: Shinya Kato <[email-masked]>
Date: Wed, 4 Jun 2025 19:05:20 +0900

> as is, given that it's syntactically correct?

Sorry.
CREATE TABLE name (...) -> CREATE UNLOGGED TABLE name (...)

-- 
Best regards,
Shinya Kato
NTT OSS Center

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 4 Jun 2025 11:15:29 -0500

> CREATE TABLE name (...) -> CREATE UNLOGGED TABLE name (...)

I see no benefit in recommending things that are guaranteed to error.  In
commit 5c1ce1b, we removed tab completion for CREATE UNLOGGED MATERIALIZED
VIEW even though it is supported by the grammar.  The partitioned table
case sounds like roughly the same situation.

-- 
nathan

---

From: Michael Paquier <[email-masked]>
Date: Thu, 5 Jun 2025 09:23:42 +0900

> case sounds like roughly the same situation.

Agreed to not suggest the PARTITION BY clause in the tab completion as
it is not supported by the backend for unlogged tables.
tab-complete.in.c has some handling for CREATE UNLOGGED TABLE around
line 3667, so we could just have an extra case for it, like in the
attached patch.  A split already exists for temporary tables to handle
the ON COMMIT clause after the attribute list.

Thoughts?
--
Michael
diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index ec65ab79fecb..620830feb9d7 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -3664,9 +3664,10 @@ match_previous_words(int pattern_id,
 			 TailMatches("CREATE", "TEMP|TEMPORARY|UNLOGGED", "TABLE", MatchAny, "(*)", "AS"))
 		COMPLETE_WITH("EXECUTE", "SELECT", "TABLE", "VALUES", "WITH");
 	/* Complete CREATE TABLE name (...) with supported options */
-	else if (TailMatches("CREATE", "TABLE", MatchAny, "(*)") ||
-			 TailMatches("CREATE", "UNLOGGED", "TABLE", MatchAny, "(*)"))
+	else if (TailMatches("CREATE", "TABLE", MatchAny, "(*)"))
 		COMPLETE_WITH("AS", "INHERITS (", "PARTITION BY", "USING", "TABLESPACE", "WITH (");
+	else if (TailMatches("CREATE", "UNLOGGED", "TABLE", MatchAny, "(*)"))
+		COMPLETE_WITH("AS", "INHERITS (", "USING", "TABLESPACE", "WITH (");
 	else if (TailMatches("CREATE", "TEMP|TEMPORARY", "TABLE", MatchAny, "(*)"))
 		COMPLETE_WITH("AS", "INHERITS (", "ON COMMIT", "PARTITION BY", "USING",
 					  "TABLESPACE", "WITH (");

---

From: Shinya Kato <[email-masked]>
Date: Thu, 5 Jun 2025 10:04:23 +0900

> the ON COMMIT clause after the attribute list.

Thank you. It looks good to me.

-- 
Best regards,
Shinya Kato
NTT OSS Center

---

From: Michael Paquier <[email-masked]>
Date: Thu, 5 Jun 2025 13:57:30 +0900

> Thank you. It looks good to me.

How does the RMT feel about this change?  Nathan, would you be OK with
that?  It's not a big problem, as well, if the code is kept as-is, but
as it's a simple change..
--
Michael

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 5 Jun 2025 15:04:45 -0500

> as it's a simple change..

IMHO a case can be reasonably made that this is an oversight in the related
commit.  I've added the rest of the RMT here in case they see it
differently.

The patch LGTM, too.

-- 
nathan

---

From: Michael Paquier <[email-masked]>
Date: Fri, 6 Jun 2025 10:55:32 +0900

> commit.  I've added the rest of the RMT here in case they see it

Okay, thanks.  Let's wait for a couple of days in case there are more
opinions and/or comments.  I propose to apply the patch around the
beginning of next week if there are no objections.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Wed, 11 Jun 2025 09:28:13 +0900

> beginning of next week if there are no objections.

It took a bit longer than wanted, but applied now.
--
Michael