Re: [HACKERS] Radix tree for character conversion - Mailing list pgsql-hackers

At Wed, 1 Mar 2017 14:34:23 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqQ_4n+FDWi5Xiueo68i=fTmdg1Wx+y6XWWX=8rAhKRtFw@mail.gmail.com>
> On Tue, Feb 28, 2017 at 5:34 PM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> > At Tue, 28 Feb 2017 15:20:06 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqR49krGP6qaaKaL2v3HCnn+dnzv8Dq_ySGbDSr6b_ywrw@mail.gmail.com>
> >> +conv.o: conv.c char_converter.c
> >> This also can go away.
> >
> > Touching char_converter.c will be ignored if it is removed. Did
> > you mistake it for map_checker?
> 
> That was not what I meant: as pg_mb_radix_conv() is only used in
> conv.c, it may be better to just remove completely char_converter.c.

Ouch! You're right. Sorry for my short-sight. char_converter.c is
removed and related description in Makefile is removed.

> > And the code-comment pointed in the comment by the previous mail
> > is rewritten as Robert's suggestion.
> 
> Fine for me.
> 
> -distclean: clean
> +distclean:
>     rm -f $(TEXTS)
> 
> -maintainer-clean: distclean
> -   rm -f $(MAPS)
> -
> +maintainer-clean:
> +   rm -f $(TEXTS) $(MAPS)
> Well, I would have assumed that this should not change..

I should have reverted there but actually the patch somehow does
the different thing.. Surely reverted it this time.

> The last version of the patch looks in rather good shape to me, we are
> also sure that the sanity checks on the old maps and the new maps
> match per the previous runs with map_checker.

Agreed.

>  One thing that still
> need some extra opinions is what to do with the old maps:
> 1) Just remove them, replacing the old maps by the new radix tree maps.
> 2) Keep them around in the backend code, even if they are useless.
> 3) Use a GUC to be able to switch from one to the other, giving a
> fallback method in case of emergency.
> 4) Use an extension module to store the old maps with as well the
> previous build code, so as sanity checks can still be performed on the
> new maps.
> 
> I would vote for 2), to reduce long term maintenance burdens and after
> seeing all the sanity checks that have been done in previous versions.

I don't vote 3 and 4. And I did 1 in the last patch.

About 2, any change in the authority files rarely but possiblly
happens. Even in the case the plain map files are no longer can
be generated. (but can with a bit tweak of convutils.pm) If the
radix-tree file generator is under a suspicion, the "plain" map
file generator (and the map_checker) or some other means to
sanity check might be required.

That being said, when something occurs in radix files, we can
find it in the radix file and can find the corresponding lines in
the aurhority file. The remaining problem is the case where some
substantial change in authority files doesn't affect radix
files. We can detect such mistake by detecting changes in
authority files. So I propose the 5th option.

5) Just remove plain map files and all related code. Addition to  that, Makefile stores hash digest of authority files
in Unicode/authoriy_hashes.txt or something that is managed by  git.
 

This digest may differ among platforms (typically from cr/nl
difference) but we can assume *nix for the usage.

I will send the next version after this discussion is settled.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center