From: Zsolt Parragi <zsolt.parragi@percona.com>
In-Reply-To: 
 <CANxoLDco7z7Fj7_pj_3YQLvjq8ZH2Ai4c3GcOamed01fZhJ77Q@mail.gmail.com>
References: 
 <CANxoLDfjQnhM=E6JSyYo9s9OdjqoN8s_3wE5yL=kaDu_X8j-dA@mail.gmail.com>
 <CANxoLDcxmXNHNyPt5v+LaBJVJmyt=j95D7TDE9SepoM7y15t-w@mail.gmail.com>
 <CAN4CZFNqf41TCg1ENccq6DAYQpArCbnKUSidHnRKaVMWGj5pBA@mail.gmail.com>
 <CANxoLDfL1AxL=k9SsRNReKQ-sJUSy6RvCqeog5HWckFxff=0Pg@mail.gmail.com>
 <CAN4CZFP5-fAGwxMCyKGePgp3N8NCpoFXkruO=5xjTC=gPwqYxw@mail.gmail.com>
 <CANxoLDc6UFMsyzbXKF8SpMoXdoKE=-yRPqtvyBMKKJ2zOUKX4A@mail.gmail.com>
 <CAN4CZFMhnZyQsdLC5iZNego77JHJyP6J_4CpWu=99vKHCd-vzg@mail.gmail.com>
 <CANxoLDco7z7Fj7_pj_3YQLvjq8ZH2Ai4c3GcOamed01fZhJ77Q@mail.gmail.com>
MIME-Version: 1.0
Date: Thu, 11 Jun 2026 14:00:59 -0700
Message-ID: 
 <CAN4CZFP8JTUGvYJAk1QoFB7xmqQTmc72_FOe38qsE9S=CcYD4g@mail.gmail.com>
Subject: Re: [PATCH] Add pg_get_table_ddl() to reconstruct CREATE TABLE
 statements
To: pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAN4CZFP8JTUGvYJAk1QoFB7xmqQTmc72_FOe38qsE9S%3DCcYD4g%40mail.gmail.com>
Precedence: bulk

The new version seem to work correctly to me, I didn't find any
further issues. Now the main blockers seem to be the remaining TODOs
related to includes_triggers/includes_policies.

I only have some minor comments about code structuring:

+ * get_inheritance_parents
+ *		Return a List of parent OIDs for relid, ordered by inhseqno.
+ *
+ * find_inheritance_children() walks the opposite direction (parent->children),

Shouldn't this follow the same naming and parameter pattern and live
at the same place in pg_inherits?

+static char *
+lookup_qualified_relname(Oid relid)
+...
+static char *
+lookup_relname_for_emit(Oid relid, bool schema_qualified, Oid base_namespace)

Is lookup_qualified_relname needed? It is only called within
lookup_relname_for_emit, and it results in a double syscache lookup,
which could be avoided if these were a single function.

+		/* COMPRESSION clause, only if explicitly set on the column. */
+		if (CompressionMethodIsValid(att->attcompression))
+		{
+			const char *cm = NULL;
+
+			switch (att->attcompression)
+			{
+				case TOAST_PGLZ_COMPRESSION:
+					cm = "pglz";
+					break;
+				case TOAST_LZ4_COMPRESSION:
+					cm = "lz4";
+					break;
+			}
+			if (cm)
+				appendStringInfo(buf, " COMPRESSION %s", cm);
+		}

Isn't this basically GetCompressionMethodName(att->attcompression)?

+		/* STORAGE clause, only if it differs from the type's default. */
+		if (att->attstorage != get_typstorage(att->atttypid))
+		{
+			const char *storage = NULL;
+
+			switch (att->attstorage)
+			{
+				case TYPSTORAGE_PLAIN:
+...

And this seems like storage_name(att->attstorage) from tablecmds.c,
the only issue is that that's currently static