Hi, Andrew
On Tue, 31 Mar 2026 at 15:42, Andrew Dunstan <[email protected]> wrote: > On 2026-03-24 Tu 5:56 PM, Zsolt Parragi wrote: >> v2 is definitely better, I can confirm the improvements. >> >>> including all suggested changes in this thread >> But I want to point out that the permission check question, and the >> role membership question is still open. >> >> For the membership question >> >>> I'm not opposed >>> to your idea but IMO it should be an option. >> I'm also fine with leaving it out, but then it should be a mentioned >> limitation. >> >> For v2: >> >> Shouldn't the tablespace function also support an owner option >> similarly to the database function? >> >> A pfree(nulls) and pfree(spcname) seem to be missing, all other >> variables are freed properly. >> >> + >> + /* >> + * Variables that are marked GUC_LIST_QUOTE were already fully >> + * quoted before they were put into the setconfig array. Break >> + * the list value apart and then quote the elements as string >> + * literals. >> + */ >> + if (GetConfigOptionFlags(s, true) & GUC_LIST_QUOTE) >> + { >> >> This part seems to be duplicated between the two functions, maybe >> could be a helper? >> >> >> +ALTER ROLE regress_role_ddl_test4 SET search_path TO 'myschema, public'; >> >> Maybe it would be useful to also test the "SET search_path TO >> myschema, public" variant, without quotes? >> >> >> I also want to go back to the datestyle question one more time: >> >>> The non-fixed DateStyle is by design. It mimics pg_dumpall. >> Isn't pg_dump and pg_dumpall inconsistent in this? pg_dump sets it to >> ISO, pg_dumpall uses DateStyle. So I'm not that sure about the >> "pg_dumpall works this way" argument because pg_dump works >> differently. Maybe either of those tools should also be fixed? >> >> The pg_dump behavior is actually a bugfix in cf4cee1b, which was never >> applied to pg_dumpall, so it seems like an oversight to me? >> >>> At present, dates are put into a dump in the format specified by the >>> default datestyle. This is not portable between installations. >>> >>> This patch sets DATESTYLE to ISO at the start of a pg_dump, so that the >>> dates written into the dump will be restorable onto any database, >>> regardless of how its default datestyle is set. > > > OK, I hope the attached set addresses all the outstanding > issues. We're using ISO dates, and there are appropriate permissions > checks. There is an option to dump role memberships. > > Thanks for updating the v3 patches. Here are some comments. v3-0001 ======= 1. + List *namelist; + bool first = true; + + /* Parse string into list of identifiers */ + if (!SplitGUCList(rawval, ',', &namelist)) According to SplitGUCList()'s comment, the caller should call list_free() on the returned list even on error. Should we also call list_free() on namelist? v3-0003 ======= 1. + /* Add OWNER clause */ + if (!no_owner) + { + spcowner = GetUserNameFromId(tspForm->spcowner, false); + append_ddl_option(&buf, pretty, 4, "OWNER %s", + quote_identifier(spcowner)); + pfree(spcowner); + } The spcowner is only used within the if scope, so we can narrow its scope. v3-0004 ======== 1. + append_ddl_option(&buf, pretty, 4, "WITH TEMPLATE = template0"); I'm curious why WITH TEMPLATE = template0 is hardcoded. For example: [local]:1374846 postgres=# create database db01 IS_TEMPLATE true; CREATE DATABASE [local]:1374846 postgres=# create database db02 template db01; CREATE DATABASE [local]:1374846 postgres=# select pg_get_database_ddl('db02'); pg_get_database_ddl ----------------------------------------------------------------------------------------------------------------- CREATE DATABASE db02 WITH TEMPLATE = template0 ENCODING = 'UTF8' LOCALE_PROVIDER = libc LOCALE = 'en_US.UTF-8'; ALTER DATABASE db02 OWNER TO japin; (2 rows) Is this working as expected? It seems there's no way to reconstruct the WITH TEMPLATE clause, right? A comment here would help. > cheers > > > andrew > > > -- > Andrew Dunstan > EDB: https://www.enterprisedb.com > > [2. text/x-patch; > v3-0001-Add-infrastructure-for-pg_get_-_ddl-functions.patch]... > > [3. text/x-patch; v3-0002-Add-pg_get_role_ddl-function.patch]... > > [4. text/x-patch; v3-0003-Add-pg_get_tablespace_ddl-function.patch]... > > [5. text/x-patch; v3-0004-Add-pg_get_database_ddl-function.patch]... -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
