Обсуждение: doc examples for pghandler
Hi everyone, Would some additional procedure language handler code examples in the documentation be good to add? I've put some together in the attached patch, and can log it to a future commitfest if people think it would be a good addition. Regards, Mark -- Mark Wong 2ndQuadrant - PostgreSQL Solutions for the Enterprise https://www.2ndQuadrant.com/
Вложения
Mark Wong <mark@2ndquadrant.com> writes:
> Would some additional procedure language handler code examples in the
> documentation be good to add? I've put some together in the attached
> patch, and can log it to a future commitfest if people think it would
> be a good addition.
Hmm. The existing doc examples are really pretty laughable, because
there's such a large gap between the offered skeleton and a workable
handler. So I agree it'd be nice to do better, but I'm suspicious of
having large chunks of sample code in the docs --- that's a maintenance
problem, if only because we likely won't notice when we break it.
Also, if somebody is hoping to copy-and-paste such code, it isn't
that nice to work from if it's embedded in SGML.
I wonder if it'd be possible to adapt what you have here into some
tiny contrib module that doesn't do very much useful, but can at
least be tested to see that it compiles; moreover it could be
copied verbatim to serve as a starting point for a new PL.
regards, tom lane
On Fri, Jun 12, 2020 at 03:10:20PM -0400, Tom Lane wrote: > Mark Wong <mark@2ndquadrant.com> writes: > > Would some additional procedure language handler code examples in the > > documentation be good to add? I've put some together in the attached > > patch, and can log it to a future commitfest if people think it would > > be a good addition. > > Hmm. The existing doc examples are really pretty laughable, because > there's such a large gap between the offered skeleton and a workable > handler. So I agree it'd be nice to do better, but I'm suspicious of > having large chunks of sample code in the docs --- that's a maintenance > problem, if only because we likely won't notice when we break it. > Also, if somebody is hoping to copy-and-paste such code, it isn't > that nice to work from if it's embedded in SGML. > > I wonder if it'd be possible to adapt what you have here into some > tiny contrib module that doesn't do very much useful, but can at > least be tested to see that it compiles; moreover it could be > copied verbatim to serve as a starting point for a new PL. I do have the code examples in a repo. [1] The 0.4 directory consists of everything the examples show. It would be easy enough to adapt that for contrib, and move some of the content from the doc patch into that. Then touch up the handler chapter to reference the contrib module. Does that sound more useful? [1] https://gitlab.com/markwkm/yappl -- Mark Wong 2ndQuadrant - PostgreSQL Solutions for the Enterprise https://www.2ndQuadrant.com/
Mark Wong <mark@2ndquadrant.com> writes:
> On Fri, Jun 12, 2020 at 03:10:20PM -0400, Tom Lane wrote:
>> I wonder if it'd be possible to adapt what you have here into some
>> tiny contrib module that doesn't do very much useful, but can at
>> least be tested to see that it compiles; moreover it could be
>> copied verbatim to serve as a starting point for a new PL.
> I do have the code examples in a repo. [1] The 0.4 directory consists
> of everything the examples show.
> It would be easy enough to adapt that for contrib, and move some of the
> content from the doc patch into that. Then touch up the handler chapter
> to reference the contrib module.
On second thought, contrib/ is not quite the right place, because we
typically expect modules there to actually get installed, meaning they
have to have at least some end-user usefulness. The right place for
a toy PL handler is probably src/test/modules/; compare for example
src/test/modules/test_parser/, which is serving quite the same sort
of purpose as a skeleton text search parser.
regards, tom lane
On Fri, Jun 12, 2020 at 10:13:41PM -0400, Tom Lane wrote: > On second thought, contrib/ is not quite the right place, because we > typically expect modules there to actually get installed, meaning they > have to have at least some end-user usefulness. The right place for > a toy PL handler is probably src/test/modules/; compare for example > src/test/modules/test_parser/, which is serving quite the same sort > of purpose as a skeleton text search parser. +1 for src/test/modules/, and if you can provide some low-level API coverage through this module, that's even better. -- Michael
Вложения
On Sat, Jun 13, 2020 at 01:19:17PM +0900, Michael Paquier wrote: > On Fri, Jun 12, 2020 at 10:13:41PM -0400, Tom Lane wrote: > > On second thought, contrib/ is not quite the right place, because we > > typically expect modules there to actually get installed, meaning they > > have to have at least some end-user usefulness. The right place for > > a toy PL handler is probably src/test/modules/; compare for example > > src/test/modules/test_parser/, which is serving quite the same sort > > of purpose as a skeleton text search parser. > > +1 for src/test/modules/, and if you can provide some low-level API > coverage through this module, that's even better. Sounds good to me. Something more like the attached patch? Regards, Mark -- Mark Wong 2ndQuadrant - PostgreSQL Solutions for the Enterprise https://www.2ndQuadrant.com/
Вложения
On Sun, Jun 14, 2020 at 08:45:17PM -0700, Mark Wong wrote: > Sounds good to me. Something more like the attached patch? That's the idea. I have not gone in details into what you have here, but perhaps it would make sense to do a bit more and show how things are done in the context of a PL function called in a trigger? Your patch removes from the docs a code block that outlined that. -- Michael
Вложения
On Mon, Jun 15, 2020 at 04:47:01PM +0900, Michael Paquier wrote: > On Sun, Jun 14, 2020 at 08:45:17PM -0700, Mark Wong wrote: > > Sounds good to me. Something more like the attached patch? > > That's the idea. I have not gone in details into what you have here, > but perhaps it would make sense to do a bit more and show how things > are done in the context of a PL function called in a trigger? Your > patch removes from the docs a code block that outlined that. 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. :) Regards, Mark [1] https://commitfest.postgresql.org/29/2678/ -- Mark Wong 2ndQuadrant - PostgreSQL Solutions for the Enterprise https://www.2ndQuadrant.com/
Вложения
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. -- Michael
Вложения
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/
Вложения
On Mon, Aug 17, 2020 at 04:30:07PM -0700, Mark Wong wrote: > I've attached a small word diff to suggest a few different words to use > in the README, if that sounds better? Sounds good to me. So applied with those changes. It is really tempting to add an example of validator (one simple thing would be to return an error if trying to use TRIGGEROID or EVTTRIGGEROID), but that may not be the best thing to do for a test module. And what we have here is already much better than the original docs. -- Michael