Re: Using Expanded Objects other than Arrays from plpgsql - Mailing list pgsql-hackers

Michel Pelletier <pelletier.michel@gmail.com> writes:
> Here's a WIP patch for a pgexpanded example in src/test/modules.  The
> object is very simple and starts with an integer and increments that value
> every time it is expanded.  I added some regression tests that test two sql
> functions that replicate the expansion issue that I'm seeing with my
> extension.

I took a look through this and have a few comments:

* I really don't like the "increments when expanded" behavior.  That's
just insane from a semantic viewpoint: expanding and then flattening
a value should not change it, at least not in any user-visible way.
I get that you want the tests to exhibit how many times expansion
happened, but the LOGF() debug printouts seem to serve that need just
fine.  How about we just make the objects store a palloc'd string,
or some such?

* context_callback_exobj_free() seems bizarre.  I can see the
usefulness of the LOGF() printout for the test's purposes, but there's
no need for the pfree() because the whole context is about to go away.
I'm concerned that people using this as a skeleton might think they
need to do something equivalent, when they don't.

* Maybe the answer to the above objection is to make the expanded
object hold a malloc'd not palloc'd string, so that it's actually
necessary to have an explicit free() to avoid a permanent memory
leak.  This'd be silly in a real use-case, and should be
documented as such; but it seems good to have a callback function
to demonstrate how to clean up resources outside the expanded
object itself.

* BTW, it might be good to make LOGF() print more than just the
function's name; some indication of what it's been called on
could be very valuable.

* If we believe that this is a skeleton other people might
use as a starting point, it'd be good to have more comments
explaining what each bit is doing and why.  For instance,
I think it's important to note that a context callback function
isn't required if deleting the memory context is sufficient
to clean up the object.

* It seems like test_expand() and test_expand_expand() belong
to the pgexpanded.sql test script and should be defined there,
rather than being part of the extension.

* As the patch stands, the module would never be invoked by
"make check", other than by manually invoking that in the
new subdirectory.  You need to hook it into the parent
directory's Makefile.  And you need meson infrastructure
too, ie a meson.build file.  (Should be able to mostly crib
that from one of the sibling test modules.)

* This does not seem like a great idea to me:
   +SET client_min_messages = 'debug1';
It's pure luck if the test output doesn't get messed up by
somebody else's unrelated debug output, because there's
elog(DEBUG1) in a lot of places and more could show up any day.
I'd be inclined to make LOGF() emit messages at NOTICE level,
and then the test doesn't have to mess with client_min_messages
at all.

* There's a whole bunch of minor ways in which this doesn't
conform to project style:

- We don't use markdown (.md) for per-directory README files.
  (There's been discussion of that, but we're not there today.)
  I would not use a URL to cross-reference our docs, either.

- "create or replace function" isn't great style in test
  scripts, much less extension scripts.  We're not expecting
  conflicting functions to exist, and if one does it's better
  to error out than silently overwrite it.

- We put '#include "postgres.h"' in .c files, never in .h files.

- At least the .c and .h files should carry standard header
  comments with a PGDG copyright notice.

- Code layout and comments are frequently not per style.
  You can mostly fix this by running the .c and .h
  files through pgindent, but it might mangle your comments;
  usually best to make those look like project standard first.
  See https://www.postgresql.org/docs/devel/source-format.html

- Declaring static functions in a .h file doesn't seem like
  a great plan.  If the thing is only meant to be included in
  one .c file, why bother with a separate .h file at all?
  "static const ExpandedObjectMethods exobj_methods" belongs
  there even less, since you'd end with one copy per
  including file.

- We don't put "/* Local Variables: */" comments into code
  files; we generally expect the source files to be agnostic
  about what editors people use on them.


To move forward, I'd suggest dealing with these concerns first,
and then as a separate patch start adding things like
in-place-modification support.  It'd be cool to see the
results of that optimization manifest as fewer expansions
visible in the test's output.

            regards, tom lane