Обсуждение: Should we rename amapi.h and amapi.c?

Hi all,

I was working on some stuff for table AMs, and I got to wonder it we
had better rename amapi.h to indexam.h and amapi.c to indexam.c, so as
things are more consistent with table AM.  It is a bit annoying to
name the files dedicated to index AMs with what looks like now a too
generic name.  That would require switching a couple of header files
for existing module developers, which is always annoying, but the move
makes sense thinking long-term?

Any thoughts?
--
Michael

On Mon, Dec 23, 2019 at 02:34:34PM +0900, Michael Paquier wrote:
> Hi all,
> 
> I was working on some stuff for table AMs, and I got to wonder it we
> had better rename amapi.h to indexam.h and amapi.c to indexam.c, so as
> things are more consistent with table AM.  It is a bit annoying to
> name the files dedicated to index AMs with what looks like now a too
> generic name.  That would require switching a couple of header files
> for existing module developers, which is always annoying, but the move
> makes sense thinking long-term?

+1 for being more specific about which AM we're talking about.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Mon, Dec 23, 2019 at 12:28:36PM -0800, Ashwin Agrawal wrote:
> I had raised the same earlier and [1] has response from Andres, which was
> "We probably should rename it, but not in 12..."
>
> [1]
> https://www.postgresql.org/message-id/20190508215135.4eljnhnle5xp3jwb%40alap3.anarazel.de

Okay, glad to see that this has been mentioned.  So let's do some
renaming for v13 then.  I have studied first if we had better remove
amapi.c, then move amvalidate() to amvalidate.c and the handler lookup
routine to indexam.c as it already exists, but keeping things ordered
as they are makes sense to limit spreading too much dependencies with
the syscache mainly, so instead the attached patch does the following
changes:
- amapi.h -> indexam.h
- amapi.c -> indexamapi.c.  Here we have an equivalent in access/table/
as tableamapi.c.
- amvalidate.c -> indexamvalidate.c
- amvalidate.h -> indexamvalidate.h
- genam.c -> indexgenam.c

Please note that we have also amcmds.c and amcmds.c in the code, but
the former could be extended to have utilities for table AMs, and the
latter applies to both, so they are better left untouched in my
opinion.
--
Michael

On Tue, Dec 24, 2019 at 3:57 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Dec 23, 2019 at 12:28:36PM -0800, Ashwin Agrawal wrote:
> > I had raised the same earlier and [1] has response from Andres, which was
> > "We probably should rename it, but not in 12..."
> >
> > [1]
> > https://www.postgresql.org/message-id/20190508215135.4eljnhnle5xp3jwb%40alap3.anarazel.de
>
> Okay, glad to see that this has been mentioned.  So let's do some
> renaming for v13 then.  I have studied first if we had better remove
> amapi.c, then move amvalidate() to amvalidate.c and the handler lookup
> routine to indexam.c as it already exists, but keeping things ordered
> as they are makes sense to limit spreading too much dependencies with
> the syscache mainly, so instead the attached patch does the following
> changes:
> - amapi.h -> indexam.h
> - amapi.c -> indexamapi.c.  Here we have an equivalent in access/table/
> as tableamapi.c.
> - amvalidate.c -> indexamvalidate.c
> - amvalidate.h -> indexamvalidate.h
> - genam.c -> indexgenam.c
>
> Please note that we have also amcmds.c and amcmds.c in the code, but
> the former could be extended to have utilities for table AMs, and the
> latter applies to both, so they are better left untouched in my
> opinion.

Looks good to me.  There are still references to amapi.c in various
.po files, but those should rather be taken care of with the next
update-po cycle right?

On Tue, Dec 24, 2019 at 09:32:23AM +0100, Julien Rouhaud wrote:
> Looks good to me.  There are still references to amapi.c in various
> .po files, but those should rather be taken care of with the next
> update-po cycle right?

Yes, these are updated as part of the translation updates.
--
Michael

Bonjour Michaël,

> the syscache mainly, so instead the attached patch does the following
> changes:
> - amapi.h -> indexam.h
> - amapi.c -> indexamapi.c.  Here we have an equivalent in access/table/
> as tableamapi.c.
> - amvalidate.c -> indexamvalidate.c
> - amvalidate.h -> indexamvalidate.h
> - genam.c -> indexgenam.c
>

Patch applies cleanly, compiles, make check-world ok.

The change does not attempt to keep included files in ab order. Should it 
do that, or is it fixed later by some reindentation phase?

-- 
Fabien.

On Tue, Dec 24, 2019 at 02:22:22PM +0100, Fabien COELHO wrote:
> The change does not attempt to keep included files in ab order. Should it do
> that, or is it fixed later by some reindentation phase?

Yeah, it should.  Committed after fixing all that stuff.
--
Michael

Hi,

(Moving discussion from [1] to this thread)

On 2019-12-28 11:32:26 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2019-12-27 08:20:17 +0900, Michael Paquier wrote:
> >> Hm, I am not sure that it is actually that much used, such stuff is
> >> very specialized.
> 
> > That's true for some of this, but e.g. genam.h is pretty widely
> > included. I mean, you had to adapt like 100+ files and while like 30 or
> > so of those are in implementation details of individual indexes, the
> > rest is not.
> 
> This may suggest that we should think about an actual refactoring,
> rather than just mechanical renaming.  Do these results mean that
> we've allowed index API details to bleed into the wrong places?

I think the biggest API bleed is systable_* - that's legitimately needed
in a lot of places. But not actually appropriately a part of
"generalized index access method definitions.".

Furthermore I think genam.h suffers from trying to provide somewhat
distinct sets of interfaces:
- general handling of indexes: index_open/close ...
- index scan implementation: index_beginscan, ...
  index_parallelscan_initialize, ...
- systable scan implementation: systable_*
- low level index interaction helpers: IndexBuildResult, IndexVacuumInfo,
- index implementation helpers: index_store_float8_orderby_distances, ...

Now obviously we'd not want to split things quite that granular, but it
does seem like separating out external interface, systable_*, and AM
oriented things into a header each would make some sense.

Greetings,

Andres Freund

[1] https://postgr.es/m/18016.1577550746%40sss.pgh.pa.us