Re: [PATCH] Add pg_get_tablespace_ddl() function to reconstruct CREATE TABLESPACE statement - Mailing list pgsql-hackers

On 2025-Nov-05, Nishant Sharma wrote:

> My reasons why I thought only name form was sufficient:-
> 1. The use case that I had in my mind for this get DDL function was
> getting covered with name as its parameter. As we are creating DDL
> and name will be part of it. Hence using it as input to our function to
> create its DDL.

Accepting an OID as parameter lets the user call this function in a
query that returns a tablespace OID as parameter, without having to add
a join to pg_tablespace.  Not something you see very frequently, but I
can imagine GUI tool writers doing that.

> 4. The list of other tablespaces functions shared by Jim has two
> functions, pg_tablespace_location() & pg_tablespace_databases()
> that takes only oid as parameter and not name or text (maybe would
> have been better), why? I am not sure, maybe the use case at that
> time needed only an oid variant?

Lack of the other form of pg_tablespace_location has annoyed me on
occassion, but I don't think it's frequent enough to request it to be
added.  (I don't remember ever having a need to call
pg_tablespace_databases).

> +    /* Find tablespace directory path */
> +    datumlocation = DirectFunctionCall1(pg_tablespace_location, tspaceoid);
> +    path = text_to_cstring(DatumGetTextP(datumlocation));

It seems worth splitting pg_tablespace_location in two parts: one outer
shell that takes PG_FUNCTION_ARGS and returns text, and an inner
implementation function that takes a plain Oid and returns char *.  This
way, you can use the inner one here without the DirectFunctionCall1()
scaffolding, and avoid having to convert the laboriously constructed
text immediately back to a C string.

> +Datum
> +pg_get_tablespace_ddl_name(PG_FUNCTION_ARGS)
> +{
> +    PG_RETURN_TEXT_P(build_tablespace_ddl(get_tablespace_oid(NameStr(*(Name)PG_GETARG_NAME(0)),
> +                                                             false)));
> +}

This line is far too clever.  Better add a Name variable for
PG_GETARG_NAME(), an Oid variable for get_tablespace_oid(), and so on.
It'll be more code lines, but they will be ten times more readable.

> +Datum
> +pg_get_tablespace_ddl_oid(PG_FUNCTION_ARGS)
> +{
> +    PG_RETURN_TEXT_P(build_tablespace_ddl((Oid)PG_GETARG_OID(0)));
> +}

This one isn't _that_ bad -- still, our style is to have all the
PG_GETARG_foo() invocations together in an orderly fashion at the very
top of local variable declarations in pretty much all of our functions,
and I think that's a good habit to keep.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Digital and video cameras have this adjustment and film cameras don't for the
same reason dogs and cats lick themselves: because they can."   (Ken Rockwell)