Обсуждение: make tuplestore helper function

Attached is a patch to create a helper function which creates a
tuplestore to be used by FUNCAPI-callable functions.

It was suggested in [1] that I start a separate thread for this patch.

A few notes:

There are a few places with very similar code to the new helper
(MakeTuplestore()) but which, for example, don't expect the return type
to be a row (pg_ls_dir(), pg_tablespace_databases(), text_to_table()). I
wasn't sure if it was worth modifying it for their benefit.

I wasn't sure if the elog() check for call result type should be changed
to an ereport().

All callers of MakeTuplestore() pass work_mem as the third parameter, so
I could just omit it and hard-code it in, but I wasn't sure that felt
right in a utility/helper function.

I inlined deflist_to_tuplestore() in foreign.c since it was billed as a
helper function but wasn't used elsewhere and it made it harder to use
MakeTuplestore() in this location.

-- melanie

[1] https://www.postgresql.org/message-id/flat/20200124195226.lth52iydq2n2uilq%40alap3.anarazel.de

On 2021-Nov-02, Melanie Plageman wrote:

> Attached is a patch to create a helper function which creates a
> tuplestore to be used by FUNCAPI-callable functions.

Looks good, and given the amount of code being removed, it seems a
no-brainer that some helper(s) is/are needed.

I think the name is a bit too generic.  How about
MakeFuncResultTuplestore()?

I think the function should not modify rsinfo -- having that as
(undocumented) side effect is weird.  I would suggest to just return the
tuplestore, and have the caller stash it in rsinfo->setResult and modify
the other struct members alongside that.  It's a couple more lines per
caller, but the code is clearer that way.

> There are a few places with very similar code to the new helper
> (MakeTuplestore()) but which, for example, don't expect the return type
> to be a row (pg_ls_dir(), pg_tablespace_databases(), text_to_table()). I
> wasn't sure if it was worth modifying it for their benefit.

Is this just because of the tupdesc?  If that's the case, then maybe the
tupdesc can be optionally passed in by caller, and when it's not, the
helper does get_call_result_type() and the tupdesc is used as output
argument.

> I wasn't sure if the elog() check for call result type should be changed
> to an ereport().

I don't see a reason for that: it's still an internal (not user-facing) error.

> All callers of MakeTuplestore() pass work_mem as the third parameter, so
> I could just omit it and hard-code it in, but I wasn't sure that felt
> right in a utility/helper function.

No opinion.  I would have used the global in the function instead of
passing it in.

> I inlined deflist_to_tuplestore() in foreign.c since it was billed as a
> helper function but wasn't used elsewhere and it made it harder to use
> MakeTuplestore() in this location.

This is a bit strange.  Does it work to pass rsinfo->expectedDesc?

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"No renuncies a nada. No te aferres a nada."

Several places have a conditional value for the first argument (randomAccess),
but your patch changes the behavior to a constant "true".  I didn't review the
patch beyond that.

> @@ -740,18 +724,14 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
> -    tupstore =
> -        tuplestore_begin_heap(rsinfo->allowedModes & SFRM_Materialize_Random,
> -                              false, work_mem);

> @@ -2701,42 +2701,13 @@ pg_hba_file_rules(PG_FUNCTION_ARGS)
> -    tuple_store =
> -        tuplestore_begin_heap(rsi->allowedModes & SFRM_Materialize_Random,
> -                              false, work_mem);

> @@ -4799,31 +4797,8 @@ pg_timezone_names(PG_FUNCTION_ARGS)
> -    randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0;
> -    tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);

> @@ -575,38 +575,12 @@ pg_ls_dir_1arg(PG_FUNCTION_ARGS)
> -    randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0;
> -    tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);

> @@ -1170,17 +1154,12 @@ pg_cursor(PG_FUNCTION_ARGS)
> -    tupstore =
> -        tuplestore_begin_heap(rsinfo->allowedModes & SFRM_Materialize_Random,
> -                              false, work_mem);

> +++ b/src/backend/utils/fmgr/funcapi.c
> +    tupstore = tuplestore_begin_heap(true, false, maxKBytes);

-- 
Justin

The attached patch contains a helper now named
MakeFuncResultTuplestore() which aims to eliminate a few lines of
redundant code that all FUNCAPI functions making tuplestores repeated.

While investigating all of these call sites and making the changes
suggested by Álvaro, I noticed that the functions making tuplestores
broke out into seven groups. For some of them, using my helper changes
what memory context the tuple descriptor is made in.

Explanation of how/why memory context tuple descriptor is in has
changed:

An earlier version of this patch changed into the per query memory
context inside of the helper function. This version does not do so but
instead asserts that the caller has changed into the appropriate memory
context.

For callers which previously did not call get_call_result_type() and
instead used a function like CreateTupleDescCopy() to create the tuple
descriptor which was used for the tuples in the tuplestore, most of them
made this copy in the per query memory context. For these callers it
seemed error-prone to switch to the per query memory context to create
the copied tuple descriptor and then switch back before calling the
helper function. Instead, the helper function will assert that the
caller has already switched into the per query memory context so that
the tuplestore is made in the per query context.

This means that for the group of functions which, prior to this patch,
called get_call_result_type() in a shorter-lived memory context but now
rely on the helper function to call get_call_result_type(), the tuple
descriptor will now be in the per query memory context.

I think that this makes sense since most of these are set as the
ResultSetInfo->setDesc (the tuple descriptor for returned tuples).

Explanation of the different groups of functions using the
MakeFuncResultTuplestore() helper:

The first group are functions which called get_call_result_type(),
allocating memory for the tuple descriptor in the short-lived memory
context, then switched into the per query memory context before
creating the tuplestore, then switched back to the short-lived memory
context directly after making the tuplestore.

Now that they use the helper function and switch into the per query
memory context before calling it, the tuple descriptor is allocated in
the per query memory context.

The first group:

  pg_stop_backup_v2()
  pg_event_trigger_dropped_objects()
  pg_event_trigger_ddl_commands()
  pg_available_extensions()
  pg_available_extension_versions()
  pg_extension_update_paths()
  pg_hba_file_rules()
  pg_stat_get_subscription()
  pg_logical_slot_get_changes_guts()
  pg_show_replication_origin_status()
  pg_get_replication_slots()
  pg_stat_get_wal_senders()
  pg_get_shmem_allocations()
  pg_timezone_names()
  pg_ls_dir_files()
  pg_get_backend_memory_contexts()
  pg_stat_get_progress_info()
  pg_stat_get_activity()
  pg_stat_get_slru()

The second group are functions which, instead of calling
get_call_result_type(), call CreateTemplateTupleDesc() and
TupleDescInitEntry() to allocate and initialize the tuple descriptor in
the per query memory context.
Their memory usage is unchanged by this patch.

The second group:

   pg_prepared_statement()
   pg_ls_dir()
   pg_tablespace_databases()
   show_all_file_settings()
   pg_cursor()

The third and fourth groups are functions that use CreateTupleDescCopy()
to make a tuple descriptor copy from ResultSetInfo->expectedDesc.
These functions made a copy of ResultSetInfo->expectedDesc but did not
check if it was present before doing so (and do not seem to call a
function which would call internal_get_result_type() -- which does this
check), so I have added that check.
Their memory usage is unchanged by this patch.

The third group:

  text_to_table()

The fourth group differs from the third in that the values for the
tuples are actually created in the per query memory context -- seemingly
unnecessarily since tuplestore_putvalues() will eventually end up
copying everything when it is forming tuples. This behavior is not
altered at all by the MakeFuncResultTuplestore() helper function patch,
however I wanted to mention that I noticed that perhaps more was being
allocated in the per query memory context than is necessary.

The fourth group:

   pg_options_to_table()
   pg_config()

The fifth group is a function that uses CreateTupleDescCoy() to make a
copy of a tuple descriptor (NOT ResultSetInfo->expectedDesc) in the
function info memory context (fcinfo->flinfo->fn_mcxt) so that it can be
accessed later.
Their memory usage is unchanged by this patch.

The fifth group:

   populate_recordset_worker()

The sixth group of functions use CreateTupleDescCopy() to make a copy of
the tuple descriptor that they set up using get_call_result_type().

They called get_call_result_type() in a short-lived memory context, then
switched to the per query memory context and made a copy of it right
away. It is unclear why they couldn't just call get_call_result_type()
in the per query context and not make a copy. Perhaps they needed some
specific behavior of the copy (like that it doesn't copy constraints and
defaults?).

With the attached patch, the initial tuple descriptor is now made in the
per query memory context (as well as the copy). I wonder if the initial
tuple descriptor being made in the per query memory context negated the
need for a copy, however I left the copy intact since I wasn't sure.

These functions also checked for the presence of
ResultSetInfo->expectedDesc which is not applicable for all callers of
MakeFuncResultTuplestore(), so I separated it into a separate check.
get_call_result_type() will call internal_get_result_type() which will
check for expectedDesc if the result type is TYPEFUNC_RECORD, so I'm not
sure if the expectedDesc check is required or not in the body of these
functions.

The sixth group:

   each_worker()
   each_worker_jsonb()

The seventh group of functions use CreateTupleDescCopy() to make a copy of
ResultSetInfo->expectedDesc in the per query memory context.
This patch does not change any of that. The only difference is that now
the check for expectedDesc is separate.
Their memory usage is unchanged by this patch.

The seventh group:

   elements_worker_jsonb()
   elements_worker()

Note that for all of the functions in which I added or changed the check for
ResultSetInfo->expectedDesc, it is a little bit different because I've changed
the error code.
I've used error code: ERRCODE_E_R_I_E_SRF_PROTOCOL_VIOLATED where they
previously would have been part of an ereport with error code
ERRCODE_FEATURE_NOT_SUPPORTED.
I'm not sure if the new one is correct. In fact, I'm not sure if an error
related to expectedDesc being set should be user-facing at all (looking at how
expectedDesc is used and set, I wasn't sure). Perhaps it should be an elog() or
an assert?

Feedback is addressed inline below.

On Tue, Nov 2, 2021 at 2:17 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Nov-02, Melanie Plageman wrote:
>
> > Attached is a patch to create a helper function which creates a
> > tuplestore to be used by FUNCAPI-callable functions.
>
> Looks good, and given the amount of code being removed, it seems a
> no-brainer that some helper(s) is/are needed.
>
> I think the name is a bit too generic.  How about
> MakeFuncResultTuplestore()?

I've done this rename.

> I think the function should not modify rsinfo -- having that as
> (undocumented) side effect is weird.  I would suggest to just return the
> tuplestore, and have the caller stash it in rsinfo->setResult and modify
> the other struct members alongside that.  It's a couple more lines per
> caller, but the code is clearer that way.

I've changed the helper not to modify rsinfo.

> > There are a few places with very similar code to the new helper
> > (MakeTuplestore()) but which, for example, don't expect the return type
> > to be a row (pg_ls_dir(), pg_tablespace_databases(), text_to_table()). I
> > wasn't sure if it was worth modifying it for their benefit.
>
> Is this just because of the tupdesc?  If that's the case, then maybe the
> tupdesc can be optionally passed in by caller, and when it's not, the
> helper does get_call_result_type() and the tupdesc is used as output
> argument.

I've parameterized the helper in this way and now am able to use it for
all FUNCAPI-callable functions making tuplestores except fmgr_sql (for
SQL functions) in functions.c because it didn't really seem worth it
given the way the code is written.

> > All callers of MakeTuplestore() pass work_mem as the third parameter, so
> > I could just omit it and hard-code it in, but I wasn't sure that felt
> > right in a utility/helper function.
>
> No opinion.  I would have used the global in the function instead of
> passing it in.

I've changed this to use the global.

> > I inlined deflist_to_tuplestore() in foreign.c since it was billed as a
> > helper function but wasn't used elsewhere and it made it harder to use
> > MakeTuplestore() in this location.
>
> This is a bit strange.  Does it work to pass rsinfo->expectedDesc?

This is addressed by changing MakeFuncResultTuplestore() call
get_call_result_type() optionally and having pg_options_to_table() call
it with NULL as tuple descriptor. Now the behavior is the same -- it
makes a copy of expectedDesc in the per query context and sets that as
setDesc in the ReturnSetInfo.

- Melanie

On Tue, Nov 2, 2021 at 4:23 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> Several places have a conditional value for the first argument (randomAccess),
> but your patch changes the behavior to a constant "true".  I didn't review the
> patch beyond that.
>
> > @@ -740,18 +724,14 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
> > -     tupstore =
> > -             tuplestore_begin_heap(rsinfo->allowedModes & SFRM_Materialize_Random,
> > -                                                       false, work_mem);
>
> > @@ -2701,42 +2701,13 @@ pg_hba_file_rules(PG_FUNCTION_ARGS)
> > -     tuple_store =
> > -             tuplestore_begin_heap(rsi->allowedModes & SFRM_Materialize_Random,
> > -                                                       false, work_mem);
>
> > @@ -4799,31 +4797,8 @@ pg_timezone_names(PG_FUNCTION_ARGS)
> > -     randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0;
> > -     tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
>
> > @@ -575,38 +575,12 @@ pg_ls_dir_1arg(PG_FUNCTION_ARGS)
> > -     randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0;
> > -     tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
>
> > @@ -1170,17 +1154,12 @@ pg_cursor(PG_FUNCTION_ARGS)
> > -     tupstore =
> > -             tuplestore_begin_heap(rsinfo->allowedModes & SFRM_Materialize_Random,
> > -                                                       false, work_mem);
>
> > +++ b/src/backend/utils/fmgr/funcapi.c
> > +     tupstore = tuplestore_begin_heap(true, false, maxKBytes);
>

I believe the patch has preserved the same behavior. All of the callers
for which I replaced tuplestore_begin_heap() which passed a variable for
the randomAccess parameter had set that variable to something which was
effectively the same as passing true -- SFRM_Materialize_Random.

- Melanie

On Mon, Nov 08, 2021 at 02:52:28PM -0500, Melanie Plageman wrote:
> On Tue, Nov 2, 2021 at 4:23 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > Several places have a conditional value for the first argument (randomAccess),
> > but your patch changes the behavior to a constant "true".  I didn't review the
> > patch beyond that.
> >
> > > @@ -740,18 +724,14 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
> > > -     tupstore =
> > > -             tuplestore_begin_heap(rsinfo->allowedModes & SFRM_Materialize_Random,
> > > -                                                       false, work_mem);
> >
> > > @@ -2701,42 +2701,13 @@ pg_hba_file_rules(PG_FUNCTION_ARGS)
> > > -     tuple_store =
> > > -             tuplestore_begin_heap(rsi->allowedModes & SFRM_Materialize_Random,
> > > -                                                       false, work_mem);
> >
> > > @@ -4799,31 +4797,8 @@ pg_timezone_names(PG_FUNCTION_ARGS)
> > > -     randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0;
> > > -     tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
> >
> > > @@ -575,38 +575,12 @@ pg_ls_dir_1arg(PG_FUNCTION_ARGS)
> > > -     randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0;
> > > -     tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
> >
> > > @@ -1170,17 +1154,12 @@ pg_cursor(PG_FUNCTION_ARGS)
> > > -     tupstore =
> > > -             tuplestore_begin_heap(rsinfo->allowedModes & SFRM_Materialize_Random,
> > > -                                                       false, work_mem);
> >
> > > +++ b/src/backend/utils/fmgr/funcapi.c
> > > +     tupstore = tuplestore_begin_heap(true, false, maxKBytes);
> 
> I believe the patch has preserved the same behavior. All of the callers
> for which I replaced tuplestore_begin_heap() which passed a variable for
> the randomAccess parameter had set that variable to something which was
> effectively the same as passing true -- SFRM_Materialize_Random.

I don't think so ?

They callers aren't passing SFRM_Materialize_Random, but rather
(allowedModes & SFRM_Materialize_Random) != 0

Where allowedModes is determined EXEC_FLAG_BACKWARD.

src/include/executor/executor.h:extern Tuplestorestate *ExecMakeTableFunctionResult(SetExprState *setexpr,
src/include/executor/executor.h-                                                    ExprContext *econtext,
src/include/executor/executor.h-                                                    MemoryContext argContext,
src/include/executor/executor.h-                                                    TupleDesc expectedDesc,
src/include/executor/executor.h-                                                    bool randomAccess);

src/backend/executor/nodeFunctionscan.c=FunctionNext(FunctionScanState *node)
src/backend/executor/nodeFunctionscan.c:             ExecMakeTableFunctionResult(node->funcstates[0].setexpr,
src/backend/executor/nodeFunctionscan.c-                                         node->ss.ps.ps_ExprContext,
src/backend/executor/nodeFunctionscan.c-                                         node->argcontext,
src/backend/executor/nodeFunctionscan.c-                                         node->funcstates[0].tupdesc,
src/backend/executor/nodeFunctionscan.c-                                         node->eflags & EXEC_FLAG_BACKWARD);

-- 
Justin

On Mon, Nov 8, 2021 at 3:13 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Mon, Nov 08, 2021 at 02:52:28PM -0500, Melanie Plageman wrote:
> > On Tue, Nov 2, 2021 at 4:23 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > >
> > > Several places have a conditional value for the first argument (randomAccess),
> > > but your patch changes the behavior to a constant "true".  I didn't review the
> > > patch beyond that.
> > >
> > > > @@ -740,18 +724,14 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
> > > > -     tupstore =
> > > > -             tuplestore_begin_heap(rsinfo->allowedModes & SFRM_Materialize_Random,
> > > > -                                                       false, work_mem);
> > >
> > > > @@ -2701,42 +2701,13 @@ pg_hba_file_rules(PG_FUNCTION_ARGS)
> > > > -     tuple_store =
> > > > -             tuplestore_begin_heap(rsi->allowedModes & SFRM_Materialize_Random,
> > > > -                                                       false, work_mem);
> > >
> > > > @@ -4799,31 +4797,8 @@ pg_timezone_names(PG_FUNCTION_ARGS)
> > > > -     randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0;
> > > > -     tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
> > >
> > > > @@ -575,38 +575,12 @@ pg_ls_dir_1arg(PG_FUNCTION_ARGS)
> > > > -     randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0;
> > > > -     tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
> > >
> > > > @@ -1170,17 +1154,12 @@ pg_cursor(PG_FUNCTION_ARGS)
> > > > -     tupstore =
> > > > -             tuplestore_begin_heap(rsinfo->allowedModes & SFRM_Materialize_Random,
> > > > -                                                       false, work_mem);
> > >
> > > > +++ b/src/backend/utils/fmgr/funcapi.c
> > > > +     tupstore = tuplestore_begin_heap(true, false, maxKBytes);
> >
> > I believe the patch has preserved the same behavior. All of the callers
> > for which I replaced tuplestore_begin_heap() which passed a variable for
> > the randomAccess parameter had set that variable to something which was
> > effectively the same as passing true -- SFRM_Materialize_Random.
>
> I don't think so ?
>
> They callers aren't passing SFRM_Materialize_Random, but rather
> (allowedModes & SFRM_Materialize_Random) != 0
>
> Where allowedModes is determined EXEC_FLAG_BACKWARD.
>
> src/include/executor/executor.h:extern Tuplestorestate *ExecMakeTableFunctionResult(SetExprState *setexpr,
> src/include/executor/executor.h-                                                    ExprContext *econtext,
> src/include/executor/executor.h-                                                    MemoryContext argContext,
> src/include/executor/executor.h-                                                    TupleDesc expectedDesc,
> src/include/executor/executor.h-                                                    bool randomAccess);
>
> src/backend/executor/nodeFunctionscan.c=FunctionNext(FunctionScanState *node)
> src/backend/executor/nodeFunctionscan.c:             ExecMakeTableFunctionResult(node->funcstates[0].setexpr,
> src/backend/executor/nodeFunctionscan.c-                                         node->ss.ps.ps_ExprContext,
> src/backend/executor/nodeFunctionscan.c-                                         node->argcontext,
> src/backend/executor/nodeFunctionscan.c-                                         node->funcstates[0].tupdesc,
> src/backend/executor/nodeFunctionscan.c-                                         node->eflags & EXEC_FLAG_BACKWARD);
>

You are right. I misread it.

So, I've attached a patch where randomAccess is now an additional
parameter (and registered for the next fest).

I was thinking about how to add a test that would have broken when I
passed true for randomAccess to tuplestore_begin_heap() when false was
required. But, I don't fully understand the problem. If backward
accesses to a tuplestore are not allowed but randomAccess is mistakenly
passed as true, would the potential result be potentially wrong results
from accessing the tuplestore results backwards?

- Melanie

On Thu, Nov 18, 2021 at 12:59:03PM -0500, Melanie Plageman wrote:
> On Mon, Nov 8, 2021 at 3:13 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > On Mon, Nov 08, 2021 at 02:52:28PM -0500, Melanie Plageman wrote:
> > > On Tue, Nov 2, 2021 at 4:23 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > >
> > > > Several places have a conditional value for the first argument (randomAccess),
> > > > but your patch changes the behavior to a constant "true".  I didn't review the
> > > > patch beyond that.
> > > >
...
> > > I believe the patch has preserved the same behavior. All of the callers
> > > for which I replaced tuplestore_begin_heap() which passed a variable for
> > > the randomAccess parameter had set that variable to something which was
> > > effectively the same as passing true -- SFRM_Materialize_Random.
> >
> > I don't think so ?
> >
> > They callers aren't passing SFRM_Materialize_Random, but rather
> > (allowedModes & SFRM_Materialize_Random) != 0
> >
> > Where allowedModes is determined EXEC_FLAG_BACKWARD.
...
> You are right. I misread it.
> 
> So, I've attached a patch where randomAccess is now an additional
> parameter (and registered for the next fest).
> 
> I was thinking about how to add a test that would have broken when I
> passed true for randomAccess to tuplestore_begin_heap() when false was
> required. But, I don't fully understand the problem. If backward
> accesses to a tuplestore are not allowed but randomAccess is mistakenly
> passed as true, would the potential result be potentially wrong results
> from accessing the tuplestore results backwards?

No, see here

src/backend/utils/fmgr/README-The Tuplestore must be created with randomAccess = true if
src/backend/utils/fmgr/README:SFRM_Materialize_Random is set in allowedModes, but it can (and preferably
src/backend/utils/fmgr/README-should) be created with randomAccess = false if not.  Callers that can support
src/backend/utils/fmgr/README-both ValuePerCall and Materialize mode will set SFRM_Materialize_Preferred,
src/backend/utils/fmgr/README-or not, depending on which mode they prefer.

If you use "randomAccess=true" when it's not needed, the result might be less
efficient.

Some callers specify "true" when they don't need to.  I ran into that here.
https://www.postgresql.org/message-id/21724.1583955158@sss.pgh.pa.us

Maybe you'd want to add an 0002 patch which changes those to conditional ?

BTW, there should be a newline before MakeFuncResultTuplestore().

-- 
Justin

On Thu, Nov 18, 2021 at 1:24 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Thu, Nov 18, 2021 at 12:59:03PM -0500, Melanie Plageman wrote:
> > On Mon, Nov 8, 2021 at 3:13 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > On Mon, Nov 08, 2021 at 02:52:28PM -0500, Melanie Plageman wrote:
> > > > On Tue, Nov 2, 2021 at 4:23 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > > >
> > > > > Several places have a conditional value for the first argument (randomAccess),
> > > > > but your patch changes the behavior to a constant "true".  I didn't review the
> > > > > patch beyond that.
> > > > >
> ...
> > > > I believe the patch has preserved the same behavior. All of the callers
> > > > for which I replaced tuplestore_begin_heap() which passed a variable for
> > > > the randomAccess parameter had set that variable to something which was
> > > > effectively the same as passing true -- SFRM_Materialize_Random.
> > >
> > > I don't think so ?
> > >
> > > They callers aren't passing SFRM_Materialize_Random, but rather
> > > (allowedModes & SFRM_Materialize_Random) != 0
> > >
> > > Where allowedModes is determined EXEC_FLAG_BACKWARD.
> ...
> > You are right. I misread it.
> >
> > So, I've attached a patch where randomAccess is now an additional
> > parameter (and registered for the next fest).
> >
> > I was thinking about how to add a test that would have broken when I
> > passed true for randomAccess to tuplestore_begin_heap() when false was
> > required. But, I don't fully understand the problem. If backward
> > accesses to a tuplestore are not allowed but randomAccess is mistakenly
> > passed as true, would the potential result be potentially wrong results
> > from accessing the tuplestore results backwards?
>
> No, see here
>
> src/backend/utils/fmgr/README-The Tuplestore must be created with randomAccess = true if
> src/backend/utils/fmgr/README:SFRM_Materialize_Random is set in allowedModes, but it can (and preferably
> src/backend/utils/fmgr/README-should) be created with randomAccess = false if not.  Callers that can support
> src/backend/utils/fmgr/README-both ValuePerCall and Materialize mode will set SFRM_Materialize_Preferred,
> src/backend/utils/fmgr/README-or not, depending on which mode they prefer.
>
> If you use "randomAccess=true" when it's not needed, the result might be less
> efficient.
>
> Some callers specify "true" when they don't need to.  I ran into that here.
> https://www.postgresql.org/message-id/21724.1583955158@sss.pgh.pa.us
>
> Maybe you'd want to add an 0002 patch which changes those to conditional ?

Done in attached v4

> BTW, there should be a newline before MakeFuncResultTuplestore().

Fixed

- Melanie

On Mon, Dec 13, 2021 at 05:27:52PM -0500, Melanie Plageman wrote:
> Done in attached v4

Thanks.

I think these comments can be removed from the callers:
/* it's a simple type, so don't use get_call_result_type */

You removed one call to tuplestore_donestoring(), but not the others.
I guess you could remove them all (optionally as a separate patch).

The rest of these are questions more than comments, and I'm not necessarily
suggesting to change the patch:

This isn't new in your patch, but for me, it's more clear if the callers have a
separate boolean for randomAccess, rather than having the long expression in
the function call:

+       tupstore = MakeFuncResultTuplestore(fcinfo, NULL,
+               rsinfo->allowedModes & SFRM_Materialize_Random);

vs

        randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0;
-       tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
+       tupstore = MakeFuncResultTuplestore(fcinfo, NULL, randomAccess);

One could also argue for passing rsinfo->allowedModes, instead of
(rsinfo->allowedModes & SFRM_Materialize_Random).

There's a couples places that you're checking expectedDesc where it wasn't
being checked before.  Is that some kind of live bug ?
pg_config() text_to_table()

It'd be nice if the "expected tuple format" error didn't need to be duplicated
for each SRFs.  I suppose the helper function could just take a boolean
determining whether or not to throw an error (passing "expectedDesc==NULL").
You'd have to call the helper before CreateTupleDescCopy() - which you're
already doing in a couple places for similar reasons.

I noticed that tuplestore.h isn't included most places.  I suppose it's
included via execnodes.h.  Your patch doesn't change that, arguably it
should've always been included.

-- 
Justin

Thanks for the detailed review!

On Fri, Dec 17, 2021 at 3:04 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Mon, Dec 13, 2021 at 05:27:52PM -0500, Melanie Plageman wrote:
> > Done in attached v4
>
> Thanks.
>
> I think these comments can be removed from the callers:
> /* it's a simple type, so don't use get_call_result_type */

Done in attached v5

> You removed one call to tuplestore_donestoring(), but not the others.
> I guess you could remove them all (optionally as a separate patch).

I removed it in that one location because I wanted to get rid of the
local variable it used. I am fine with removing the other occurrences,
but I thought there might be some reason why instead of removing it,
they made it into a no-op.

> The rest of these are questions more than comments, and I'm not necessarily
> suggesting to change the patch:
>
> This isn't new in your patch, but for me, it's more clear if the callers have a
> separate boolean for randomAccess, rather than having the long expression in
> the function call:
>
> +       tupstore = MakeFuncResultTuplestore(fcinfo, NULL,
> +               rsinfo->allowedModes & SFRM_Materialize_Random);
>
> vs
>
>         randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0;
> -       tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
> +       tupstore = MakeFuncResultTuplestore(fcinfo, NULL, randomAccess);
>
> One could also argue for passing rsinfo->allowedModes, instead of
> (rsinfo->allowedModes & SFRM_Materialize_Random).

I've changed the patch to have MakeFuncResultTuplestore() check
rsinfo->allowedModes and pass in the resulting boolean to
tuplestore_begin_heap(). Because I modified the
MakeFuncResultTuplestore() function signature to accommodate this, I had
to collapse the two patches into one, as I couldn't maintain the call
sites which passed in a constant.

> There's a couples places that you're checking expectedDesc where it wasn't
> being checked before.  Is that some kind of live bug ?
> pg_config() text_to_table()

Yes, expectedDesc was accessed in these locations without checking that
it is not NULL. Should that be a separate patch?

> It'd be nice if the "expected tuple format" error didn't need to be duplicated
> for each SRFs.  I suppose the helper function could just take a boolean
> determining whether or not to throw an error (passing "expectedDesc==NULL").
> You'd have to call the helper before CreateTupleDescCopy() - which you're
> already doing in a couple places for similar reasons.

So, I don't think it makes sense to move that error message into
MakeFuncResultTuplestore() for the cases in which NULL is passed in for
the tuple descriptor. expectedDesc is not accessed at all in these cases
inside the function (since get_call_result_type()) is not called. For
the cases when a tuple descriptor is passed in, only two callers
actually check for expectedDesc -- each_worker_jsonb and each_worker.
Therefore, I don't think it makes sense to add another parameter just to
move that check inside for two callers.

> I noticed that tuplestore.h isn't included most places.  I suppose it's
> included via execnodes.h.  Your patch doesn't change that, arguably it
> should've always been included.

I've now included it in funcapi.c.

- Melanie

On Wed, Jan 05, 2022 at 12:09:16PM -0500, Melanie Plageman wrote:
> On Fri, Dec 17, 2021 at 3:04 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > The rest of these are questions more than comments, and I'm not necessarily
> > suggesting to change the patch:
> >
> > This isn't new in your patch, but for me, it's more clear if the callers have a
> > separate boolean for randomAccess, rather than having the long expression in
> > the function call:
> >
> > +       tupstore = MakeFuncResultTuplestore(fcinfo, NULL,
> > +               rsinfo->allowedModes & SFRM_Materialize_Random);
> >
> > vs
> >
> >         randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0;
> > -       tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
> > +       tupstore = MakeFuncResultTuplestore(fcinfo, NULL, randomAccess);
> >
> > One could also argue for passing rsinfo->allowedModes, instead of
> > (rsinfo->allowedModes & SFRM_Materialize_Random).
> 
> I've changed the patch to have MakeFuncResultTuplestore() check
> rsinfo->allowedModes and pass in the resulting boolean to
> tuplestore_begin_heap(). Because I modified the
> MakeFuncResultTuplestore() function signature to accommodate this, I had
> to collapse the two patches into one, as I couldn't maintain the call
> sites which passed in a constant.

> > There's a couples places that you're checking expectedDesc where it wasn't
> > being checked before.  Is that some kind of live bug ?
> > pg_config() text_to_table()
> 
> Yes, expectedDesc was accessed in these locations without checking that
> it is not NULL. Should that be a separate patch?

I don't know.  The patch is already easy to review, since it's mostly limited
to removing code and fixing inconsistencies (NULL check) and possible
inefficiencies (randomAccess).

If the expectedDesc NULL check were an 0001 patch, then 0002 (the main patch)
would be even easier to review.  Only foreign.c is different.

> > You removed one call to tuplestore_donestoring(), but not the others.
> > I guess you could remove them all (optionally as a separate patch).
> 
> I removed it in that one location because I wanted to get rid of the
> local variable it used.

What local variable ?  I see now that logicalfuncs.c never had a local variable
called tupstore.  I'm sure it was intended since 2014 (b89e15105) to say
tupestore_donestoring(p->tupstore).  But donestoring(typo) causes no error,
since the define is a NOP.

src/include/utils/tuplestore.h:#define tuplestore_donestoring(state) ((void) 0)

> I am fine with removing the other occurrences,
> but I thought there might be some reason why instead of removing it,
> they made it into a no-op.

I assume Tom left it (actually, added it back in dd04e958c) to avoid breaking
extensions for no reason.  And maybe to preserve the possbility of at some
point in the future doing something again during the "done" step.

I'd leave it for input from a committer about those:

 - remove tuplestore_donestoring() calls ?
 - split expectedDesc NULL check to an 0001 patch ?
 - anything other opened questions ?

I'm marking this as RFC, with the caveat that the newline before
MakeFuncResultTuplestore went missing again :)

-- 
Justin

On Wed, Jan 5, 2022 at 7:57 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Wed, Jan 05, 2022 at 12:09:16PM -0500, Melanie Plageman wrote:
> > On Fri, Dec 17, 2021 at 3:04 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > There's a couples places that you're checking expectedDesc where it wasn't
> > > being checked before.  Is that some kind of live bug ?
> > > pg_config() text_to_table()
> >
> > Yes, expectedDesc was accessed in these locations without checking that
> > it is not NULL. Should that be a separate patch?
>
> I don't know.  The patch is already easy to review, since it's mostly limited
> to removing code and fixing inconsistencies (NULL check) and possible
> inefficiencies (randomAccess).
>
> If the expectedDesc NULL check were an 0001 patch, then 0002 (the main patch)
> would be even easier to review.  Only foreign.c is different.

I'll wait to do that if preferred by committer.
Are you imagining that patch 0001 would only add the check for
expectedDesc that is missing from pg_config() and text_to_table()?

> > > You removed one call to tuplestore_donestoring(), but not the others.
> > > I guess you could remove them all (optionally as a separate patch).
> >
> > I removed it in that one location because I wanted to get rid of the
> > local variable it used.
>
> What local variable ?  I see now that logicalfuncs.c never had a local variable
> called tupstore.  I'm sure it was intended since 2014 (b89e15105) to say
> tupestore_donestoring(p->tupstore).  But donestoring(typo) causes no error,
> since the define is a NOP.
>
> src/include/utils/tuplestore.h:#define tuplestore_donestoring(state) ((void) 0)

Yes, I mean the local variable, tupstore.

> > I am fine with removing the other occurrences,
> > but I thought there might be some reason why instead of removing it,
> > they made it into a no-op.
>
> I assume Tom left it (actually, added it back in dd04e958c) to avoid breaking
> extensions for no reason.  And maybe to preserve the possbility of at some
> point in the future doing something again during the "done" step.
>
> I'd leave it for input from a committer about those:
>
>  - remove tuplestore_donestoring() calls ?
>  - split expectedDesc NULL check to an 0001 patch ?
>  - anything other opened questions ?
>
> I'm marking this as RFC, with the caveat that the newline before
> MakeFuncResultTuplestore went missing again :)

oops. I've attached v6 with the newline.

- Melanie

On Tue, Jan 11, 2022 at 10:19:33AM -0500, Melanie Plageman wrote:
> On Wed, Jan 5, 2022 at 7:57 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>> I'd leave it for input from a committer about those:
>>
>>  - remove tuplestore_donestoring() calls ?

This part has been raised on a different thread, as of:
https://www.postgresql.org/message-id/CAP0=ZVJeeYfAeRfmzqAF2Lumdiv4S4FewyBnZd4DPTrsSQKJKw@mail.gmail.com

And it looks sensible from here to just remove any traces of this
stuff from the C code, while leaving the compatibility macro around
to avoid any breakages with any external modules.

>>  - split expectedDesc NULL check to an 0001 patch ?

I am not sure that this is worth a split, as the same areas are
touched.

>>  - anything other opened questions ?
>>
>> I'm marking this as RFC, with the caveat that the newline before
>> MakeFuncResultTuplestore went missing again :)
>
> oops. I've attached v6 with the newline.

Saying that, the wrapper you are proposing here looks like a good
idea.  I have wanted a centralized wrapper more than once, and this is
a nice cut overall.
--
Michael

On Tue, Jan 11, 2022 at 10:19:33AM -0500, Melanie Plageman wrote:
> > If the expectedDesc NULL check were an 0001 patch, then 0002 (the main patch)
> > would be even easier to review.  Only foreign.c is different.
> 
> I'll wait to do that if preferred by committer.
> Are you imagining that patch 0001 would only add the check for
> expectedDesc that is missing from pg_config() and text_to_table()?

Yes, that's what I meant, but I haven't been able to determine if a NULL check
should be added at all.

I found this commit message, which indicates that it shouldn't be used in every
case.  I'm not clear when it should/not be used, though.

commit 60651e4cddbb77e8f1a0c7fc0be6a7e7bf626fe0
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   Sat Oct 28 14:02:21 2017 -0400

    Support domains over composite types in PL/Perl.
    
    In passing, don't insist on rsi->expectedDesc being set unless we
    actually need it; this allows succeeding in a couple of cases where
    PL/Perl functions returning setof composite would have failed before,
    and makes the error message more apropos in other cases.

On Mon, Feb 21, 2022 at 04:41:17PM +0900, Michael Paquier wrote:
> So, I got my hands on this area, and found myself applying 07daca5 as
> a first piece of the puzzle.  Anyway, after more review today, I have
> bumped into more pieces that could be consolidated, and finished with
> the following patch set:
> - 0001 changes a couple of SRFs that I found worth simplifying.  These
> refer mostly to the second and fourth group mentioned upthread by
> Melanie, with two exceptions: pg_tablespace_databases() where it is
> not worth changing to keep it backward-compatible and pg_ls_dir() as
> per its one-arg version.  That's a nice first cut in itself.
> - 0002 changes a couple of places to unify some existing SRF checks,
> that I bumped into on the way.  The value here is in using the same
> error messages everywhere, reducing the translation effort and
> generating the same errors for all cases based on the same conditions.
> This eases the review of the next patch.

These two have been now applied, with some comment improvements and
the cleanup of pg_options_to_table() done in 0001, and a slight change
in 0002 for pageinspect where a check was not necessary for a BRIN
code path.

> - 0003 is the actual refactoring meat, where I have been able to move
> the check on expectedDesc into MakeFuncResultTuplestore(), shaving
> more code than previously.  If you discard the cases of patch 0001, it
> should actually be possible to set setResult, setDesc and returnMode
> within the new function, which would feel natural as that's the place
> where we create the function's tuplestore and we want to materialize
> its contents.  The cases with the JSON code are also a bit hairy and
> need more thoughts, but we could also cut this part of the code from
> the initial refactoring effort.

This is the remaining piece, as attached, that I have not been able to
poke much at yet.
--
Michael

On Thu, Feb 24, 2022 at 08:25:06PM +0900, Michael Paquier wrote:
> This is the remaining piece, as attached, that I have not been able to
> poke much at yet.

So, I have finally poked at this last part of the patch set, and I
found that we can be more aggressive with the refactoring, by moving
into MakeFuncResultTuplestore() the parts where we save the tuplestore
and the tupledesc in the per-query memory context.  There are two
pieces that matter once things are reshaped:
- The tuple descriptor may need some extra validation via
BlessTupleDesc() when it comes from a transient record datatype,
something that happens for most of the subroutines related to the JSON
functions.
- expectedDesc is sometimes required by the caller, though most of the
time just needs to be built with the more expensive
get_call_result_type().

In order to keep things pluggable at will, MakeFuncResultTuplestore()
has been changed to access a set of bits32 flags, able to control the
two options above.  With this facility in place, I have been able to
cut much more code than the initial patch, roughly twice as of:
 24 files changed, 157 insertions(+), 893 deletions(-)

This seems in rather good shape to me, the changes are
straight-forward and the code cut is really good, so I'd like to move
on with that.  0001 is the initial patch, and 0002 is the extra
refactoring I have been working on.  The plan would be to merge both,
but I am sending a split to ease any checks on what I have changed.

Comments or objections?
--
Michael

On Mon, Feb 28, 2022 at 04:49:41PM +0900, Michael Paquier wrote:
> In order to keep things pluggable at will, MakeFuncResultTuplestore()
> has been changed to access a set of bits32 flags, able to control the
> two options above.  With this facility in place, I have been able to
> cut much more code than the initial patch, roughly twice as of:
>  24 files changed, 157 insertions(+), 893 deletions(-)

So, I have been thinking about this patch once again, and after
pondering more on it, MakeFuncResultTuplestore() is actually a wrong
name now that it does much more than just creating a tuplestore, by
assigning the TupleDesc and the TupleStore into the function's
ReturnSetInfo.

This is actually setting up a function in the context of a single call
where we fill the tuplestore with all its values, so instead I have
settled down to name that SetSingleFuncCall(), to make a parallel with
the existing MultiFuncCall*().  funcapi.c is the right place for
that, and I have added more documentation in the fmgr's README as well
as funcapi.h.
--
Michael

On Wed, Mar 02, 2022 at 03:43:17PM +0900, Michael Paquier wrote:
> This is actually setting up a function in the context of a single call
> where we fill the tuplestore with all its values, so instead I have
> settled down to name that SetSingleFuncCall(), to make a parallel with
> the existing MultiFuncCall*().  funcapi.c is the right place for
> that, and I have added more documentation in the fmgr's README as well
> as funcapi.h.

I have tortured all those code paths for the last couple of days, and
the new function name, as well as its options, still seemed fine to
me, so I have applied the patch.  The buildfarm is cool with it.  It
is worth noting that there are more code paths in contrib/ that could
be simplified with this new routine.
--
Michael

On Sun, Mar 6, 2022 at 9:29 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Mar 02, 2022 at 03:43:17PM +0900, Michael Paquier wrote:
> > This is actually setting up a function in the context of a single call
> > where we fill the tuplestore with all its values, so instead I have
> > settled down to name that SetSingleFuncCall(), to make a parallel with
> > the existing MultiFuncCall*().  funcapi.c is the right place for
> > that, and I have added more documentation in the fmgr's README as well
> > as funcapi.h.
>
> I have tortured all those code paths for the last couple of days, and
> the new function name, as well as its options, still seemed fine to
> me, so I have applied the patch.  The buildfarm is cool with it.  It
> is worth noting that there are more code paths in contrib/ that could
> be simplified with this new routine.

Wow! Thanks so much for taking the time to review, refine, and commit
this work.

I've attached a patch using the helper in most locations in contrib
modules that seemed useful.

The following I don't think we can use the helper or it is not worth it:

- pg_logdir_ls() in contrib/adminpack has return type TYPEFUNC_RECORD
  and expectedDesc is not already created, so the helper can't be used.

  But basically, since it doesn't specify OUT argument names, it has to
  do TupleDescInitEntry() itself anyway, I think.

- contrib/tablefunc.c was also not simple to refactor. the various parts
  of SetSingleFuncCall are spread throughout different functions in the
  file.

- contrib/dblink has one function which returns a tuplestore that was
  simple to change (dblink_get_notify()) and I've done so.

  However, most of the other creation of tuplestore and tupledescriptors
  is in helper functions (like materializeResult()) which separate the
  tuplestore creation from the tuple descriptor initialization in a way
  that made it hard to do a drop-in replacement with the helper function.

- Melanie

On Mon, Mar 07, 2022 at 05:09:19PM -0500, Melanie Plageman wrote:
> I've attached a patch using the helper in most locations in contrib
> modules that seemed useful.

Thanks for the patch.  I was also looking at that yesterday, and this
pretty much maps to what I have finished with, except for dblink and
xml2 where it was not looking that obvious to me at quick glance.

In xml2, my eyes hurt a bit on the meaning of doing the "Switch out of
SPI context" in xpath_table() after doing the two SPI calls, but I
agree that this extra switch back to the old context should not be
necessary.  For dblink, I did not notice that the change for
dblink_get_notify() would be this straight-forward, good catch.  It
would be nice to see prepTuplestoreResult() gone at the end, though I
am not sure how invasive/worth it would be for dblink/.

> - pg_logdir_ls() in contrib/adminpack has return type TYPEFUNC_RECORD
>   and expectedDesc is not already created, so the helper can't be used.
>   But basically, since it doesn't specify OUT argument names, it has to
>   do TupleDescInitEntry() itself anyway, I think.

Here, actually, I thought first that a simplification should be
possible, noticing that the regression tests passed with the function
called until I saw what it was testing :)

I am fine to not poke at the beast, and it looks like we may run into
trouble if we wish to maintain compatibility down to adminpack 1.0 at
runtime.  This function has a very limited usage, anyway.

> - contrib/tablefunc.c was also not simple to refactor. the various parts
>   of SetSingleFuncCall are spread throughout different functions in the
>   file.
>
> - contrib/dblink has one function which returns a tuplestore that was
>   simple to change (dblink_get_notify()) and I've done so.

This matches my impression, so applied.
--
Michael