Re: Add minimal C example and SQL registration example for custom table access methods.

2024-01-26 Thread Tristen Raab
The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   not tested
Spec compliant:   not tested
Documentation:tested, passed

Hello,

I've reviewed your patch and it applies correctly and the documentation builds 
without any error. The built documentation also looks good with no formatting 
errors. It's always great to see more examples when reading through 
documentation so I think this patch is a good addition.

thanks,

---
Tristen Raab
Highgo Software Canada
www.highgo.ca

Re: Add minimal C example and SQL registration example for custom table access methods.

2024-01-26 Thread Fabrízio de Royes Mello
On Wed, Nov 15, 2023 at 8:29 PM Roberto Mello 
wrote:
>
> Suggestion:
>
> In the C example you added you mention in the comment:
>
> +  /* Methods from TableAmRoutine omitted from example, but all
> + non-optional ones must be provided here. */
>
> Perhaps you could provide a "see " to point the reader finding your
example where he could find these non-optional methods he must provide?
>
> Nitpicking a little: your patch appears to change more lines than it
does, because it added line breaks earlier in the lines. I would generally
avoid that unless there's good reason to do so.

Hey folks,

There is a previous patch [1] around the same topic. What about joining
efforts on pointing these documentation changes to the proposed test module?

[1] https://commitfest.postgresql.org/46/4588/

-- 
Fabrízio de Royes Mello


Re: Add minimal C example and SQL registration example for custom table access methods.

2023-11-15 Thread Roberto Mello
Suggestion:

In the C example you added you mention in the comment:

+  /* Methods from TableAmRoutine omitted from example, but all
+ non-optional ones must be provided here. */

Perhaps you could provide a "see " to point the reader finding your 
example where he could find these non-optional methods he must provide?

Nitpicking a little: your patch appears to change more lines than it does, 
because it added line breaks earlier in the lines. I would generally avoid that 
unless there's good reason to do so.

Re: Add minimal C example and SQL registration example for custom table access methods.

2024-05-03 Thread Phil Eaton
Thanks Robert for mentioning this! I indeed did not notice the switch.

> Nitpicking a little: your patch appears to change more lines than it does, 
> because it added line breaks earlier in the lines. I would generally avoid 
> that unless there's good reason to do so.

Thanks! I'm not sure why that happened since I normally run
fill-region in emacs and when I re-ran it now, it looked as it used
to. I've fixed it up in this patch.

> Perhaps you could provide a "see " to point the reader finding your 
> example where he could find these non-optional methods he must provide?

Since the responses were positive, I've taken the liberty to extend
the sample code by simply including all the stub methods and the full
struct. Marking which methods are optional and not.

If that looks like too much, I can revert back. Perhaps only
mentioning the struct like we do for the index AM here:
https://www.postgresql.org/docs/current/index-api.html. However, as a
reader, I feel like the full stubs are a bit more useful.

Happy for feedback. Updated patch is attached.

Cheers,
Phil


On Fri, Mar 22, 2024 at 1:40 PM Robert Haas  wrote:
>
> On Fri, Jan 26, 2024 at 3:03 PM Fabrízio de Royes Mello
>  wrote:
> > On Wed, Nov 15, 2023 at 8:29 PM Roberto Mello  
> > wrote:
> > > Suggestion:
> > >
> > > In the C example you added you mention in the comment:
> > >
> > > +  /* Methods from TableAmRoutine omitted from example, but all
> > > + non-optional ones must be provided here. */
> > >
> > > Perhaps you could provide a "see " to point the reader finding your 
> > > example where he could find these non-optional methods he must provide?
> > >
> > > Nitpicking a little: your patch appears to change more lines than it 
> > > does, because it added line breaks earlier in the lines. I would 
> > > generally avoid that unless there's good reason to do so.
> >
> > Hey folks,
> >
> > There is a previous patch [1] around the same topic. What about joining 
> > efforts on pointing these documentation changes to the proposed test module?
> >
> > [1] https://commitfest.postgresql.org/46/4588/
>
> Looking over this thread, I see that it was moved from pgsql-docs to
> pgsql-hackers while at the same time dropping the original poster from
> the Cc list. That seems rather unfortunate. I suspect there's a pretty
> good chance that Phil Eaton hasn't seen any of the replies other than
> the first one from Paul Jungwirth, which is also the only one that
> didn't ask for anything to be changed.
>
> Re-adding Phil. Phil, you should have a look over
> https://www.postgresql.org/message-id/flat/CAByiw%2Br%2BCS-ojBDP7Dm%3D9YeOLkZTXVnBmOe_ajK%3Den8C_zB3_g%40mail.gmail.com
> and respond to the various emails and probably update the patch
> somehow. Note that feature freeze is in 2 weeks, so if we can't reach
> agreement on what is to be done here soon, this will have to wait for
> the next cycle, or later.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com


v2-0001-Add-boilerplate-C-code-and-SQL-registration-examp.patch
Description: Binary data


Re: Add minimal C example and SQL registration example for custom table access methods.

2024-05-14 Thread Robert Haas
On Fri, May 3, 2024 at 1:35 PM Phil Eaton  wrote:
> Happy for feedback. Updated patch is attached.

I took a look at this patch and I don't think this is a very good
idea, for two reasons:

1. We change the table access method interface definitions not all
that infrequently, so I think this will become out of date, and fail
to get updated.

2. Writing a table access method is really hard, and if you need this
in order to be able to attempt it, you're probably shouldn't be
attmempting it.

I wouldn't mind patching the documentation to add the SQL part of
this; that seems short enough, non-obvious enough, and sufficiently
unlikely to change that I can believe it would be a worthwhile
addition. But there have been 21 commits to tableam.h in the last 6
months and most of those would have needed to update this example, and
I think it's very likely that some of them would have forgotten it.

-- 
Robert Haas
EDB: http://www.enterprisedb.com




Re: Add minimal C example and SQL registration example for custom table access methods.

2024-05-14 Thread Phil Eaton
> I took a look at this patch and I don't think this is a very good
> idea,

No problem! I've dropped the v2 code additions and stuck with the v1
attempt plus feedback.

Thank you!

Phil


v3-0001-Add-minimal-C-example-and-SQL-registration-exampl.patch
Description: Binary data


Re: Add minimal C example and SQL registration example for custom table access methods.

2024-05-14 Thread Robert Haas
On Tue, May 14, 2024 at 3:02 PM Phil Eaton  wrote:
> > I took a look at this patch and I don't think this is a very good
> > idea,
>
> No problem! I've dropped the v2 code additions and stuck with the v1
> attempt plus feedback.

That looks more reasonable. I'd like to quibble with this text:

+. Here is an example of how to register an extension that provides a
+  table access method handler:

I think this should say something more like "Here is how an extension
SQL script might create a table access method handler". I'm not sure
if we have a standard term in our documentation that should be used
instead of "extension SQL script"; perhaps look for similar examples,
or the documentation of extensions themselves, and copy the wording.

Shouldn't "mem_tableam_handler" be "my_tableam_handler"?

-- 
Robert Haas
EDB: http://www.enterprisedb.com




Re: Add minimal C example and SQL registration example for custom table access methods.

2024-05-24 Thread Phil Eaton
> I think this should say something more like "Here is how an extension
> SQL script might create a table access method handler".

Fair point. It is referred to elsewhere [0] in docs as a "script
file", so I've done that.

> Shouldn't "mem_tableam_handler" be "my_tableam_handler"?

Sorry about that, fixed.

[0] https://www.postgresql.org/docs/current/extend-extensions.html

Phil


v4-0001-Add-minimal-C-example-and-SQL-registration-exampl.patch
Description: Binary data


Re: Add minimal C example and SQL registration example for custom table access methods.

2024-05-24 Thread Fabrízio de Royes Mello
On Fri, May 24, 2024 at 3:11 PM Phil Eaton  wrote:

> > I think this should say something more like "Here is how an extension
> > SQL script might create a table access method handler".
>
> Fair point. It is referred to elsewhere [0] in docs as a "script
> file", so I've done that.
>
> > Shouldn't "mem_tableam_handler" be "my_tableam_handler"?
>
> Sorry about that, fixed.
>
> [0] https://www.postgresql.org/docs/current/extend-extensions.html
>
> Phil
>

Nice... LGTM!

-- 
Fabrízio de Royes Mello


Re: Add minimal C example and SQL registration example for custom table access methods.

2024-07-25 Thread Tom Lane
I noticed that there were two CF entries pointing at this thread:

https://commitfest.postgresql.org/48/4655/
https://commitfest.postgresql.org/48/4973/

That doesn't seem helpful, so I've marked the second one "Withdrawn".

regards, tom lane




Re: Add minimal C example and SQL registration example for custom table access methods.

2024-03-22 Thread Robert Haas
On Fri, Jan 26, 2024 at 3:03 PM Fabrízio de Royes Mello
 wrote:
> On Wed, Nov 15, 2023 at 8:29 PM Roberto Mello  wrote:
> > Suggestion:
> >
> > In the C example you added you mention in the comment:
> >
> > +  /* Methods from TableAmRoutine omitted from example, but all
> > + non-optional ones must be provided here. */
> >
> > Perhaps you could provide a "see " to point the reader finding your 
> > example where he could find these non-optional methods he must provide?
> >
> > Nitpicking a little: your patch appears to change more lines than it does, 
> > because it added line breaks earlier in the lines. I would generally avoid 
> > that unless there's good reason to do so.
>
> Hey folks,
>
> There is a previous patch [1] around the same topic. What about joining 
> efforts on pointing these documentation changes to the proposed test module?
>
> [1] https://commitfest.postgresql.org/46/4588/

Looking over this thread, I see that it was moved from pgsql-docs to
pgsql-hackers while at the same time dropping the original poster from
the Cc list. That seems rather unfortunate. I suspect there's a pretty
good chance that Phil Eaton hasn't seen any of the replies other than
the first one from Paul Jungwirth, which is also the only one that
didn't ask for anything to be changed.

Re-adding Phil. Phil, you should have a look over
https://www.postgresql.org/message-id/flat/CAByiw%2Br%2BCS-ojBDP7Dm%3D9YeOLkZTXVnBmOe_ajK%3Den8C_zB3_g%40mail.gmail.com
and respond to the various emails and probably update the patch
somehow. Note that feature freeze is in 2 weeks, so if we can't reach
agreement on what is to be done here soon, this will have to wait for
the next cycle, or later.

-- 
Robert Haas
EDB: http://www.enterprisedb.com