On Fri, Nov 16, 2012 at 7:58 PM, Andres Freund
<andres@2ndquadrant.com> wrote:
Hi,
On 2012-11-16 13:44:45 +0900, Michael Paquier wrote:
> This patch looks OK.
>
> I got 3 comments:
> 1) Why changing the OID of pg_class_tblspc_relfilenode_index from 3171 to
> 3455? It does not look necessary.
Its a mismerge and should have happened in "Add a new RELFILENODE
syscache to fetch a pg_class entry via (reltablespace, relfilenode)" but
it seems I squashed the wrong two commits.
I had originally used 3171 but that since got used up for lo_tell64...
> 2) You should perhaps change the header of RelationMapFilenodeToOid so as
> not mentionning it as the opposite operation of RelationMapOidToFilenode
> but as an operation that looks for the OID of a relation based on its
> relfilenode. Both functions are opposite but independent.
I described it as the opposite because RelationMapOidToFilenode is the
relmappers stated goal and RelationMapFilenodeToOid is just some
side-business.
> 3) Both functions are doing similar operations. Could it be possible
> to wrap them in the same central function?
I don't really see how without making both quite a bit more
complicated. The amount of if's needed seems to be too large to me.
OK thanks for your answers.
As this patch only adds a new function and is not that much complicated, I think there is no problem in committing it. The only thing to remove is the diff in indexing.h. Could someone take care of that?
If other people have additional comments on the ability to perform a relfileoid->reloid operation for cached maps, of course go ahead.
--
Michael Paquier
http://michael.otacoo.com
