On Fri, Aug 14, 2020 at 02:25:52PM +0900, Michael Paquier wrote:
> On Tue, Aug 11, 2020 at 01:01:10PM -0700, Mark Wong wrote:
> > Ah, right. For the moment I've added some empty conditionals for
> > trigger and event trigger handling.
> >
> > I've created a new entry in the commitfest app. [1] I'll keep at it. :)
>
> Thanks for the patch. I have reviewed and reworked it as the
> attached. Some comments below.
>
> +PGFILEDESC = "PL/Sample - procedural language"
> +
> +REGRESS = create_pl create_func select_func
> +
> +EXTENSION = plsample
> +EXTVERSION = 0.1
>
> This makefile has a couple of mistakes, and can be simplified a lot:
> - make check does not work, as you forgot a PGXS part.
> - MODULES can just be used as there is only one file (forgot WIN32RES
> in OBJS for example)
> - DATA does not need the .control file.
>
> .gitignore was missing.
>
> We could just use 1.0 instead of 0.1 for the version number. That's
> not a big deal one way or another, but 1.0 is more consistent with the
> other modules.
>
> plsample--1.0.sql should complain if attempting to load the file from
> psql. Also I have cleaned up the README.
>
> Not sure that there is a point in having three different files for the
> regression tests. create_pl.sql is actually not necessary as you
> can do the same with CREATE EXTENSION.
>
> The header list of plsample.c was inconsistent with the style used
> normally in modules, and I have extended a bit the handler function so
> as we return a result only if the return type of the procedure is text
> for the source text of the function, tweaked the results a bit, etc.
> There was a family of small issues, like using ALLOCSET_SMALL_SIZES
> for the context creation. We could of course expand the sample
> handler more in the future to check for pseudotype results, have a
> validator, but that could happen later, if necessary.
Thanks for fixing all of that up for me. I did have a couple mental
notes for a couple of the last items. :)
I've attached a small word diff to suggest a few different words to use
in the README, if that sounds better?
Regards,
Mark
--
Mark Wong
2ndQuadrant - PostgreSQL Solutions for the Enterprise
https://www.2ndQuadrant.com/
