Re: [PATCH] Add pg_get_table_ddl() to reconstruct CREATE TABLE statements - Mailing list pgsql-hackers

From Zsolt Parragi
Subject Re: [PATCH] Add pg_get_table_ddl() to reconstruct CREATE TABLE statements
Date
Msg-id CAN4CZFP8JTUGvYJAk1QoFB7xmqQTmc72_FOe38qsE9S=CcYD4g@mail.gmail.com
Whole thread
In response to Re: [PATCH] Add pg_get_table_ddl() to reconstruct CREATE TABLE statements  (Akshay Joshi <akshay.joshi@enterprisedb.com>)
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Paul A Jungwirth
Date:
Subject: Re: FOR PORTION OF should reject GENERATED columns
Next
From: Tomas Vondra
Date:
Subject: Re: Key joins