On Thu, Oct 27, 2011 at 12:36 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Oct 27, 2011 at 5:37 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> On Thu, Oct 27, 2011 at 4:36 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Robert Haas <robertmhaas@gmail.com> writes:
>>>> On Wed, Oct 26, 2011 at 12:16 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>>>> This fixes both the subtrans and clog bugs in one patch.
>>>
>>>> I don't see the point of changing StartupCLOG() to be an empty
>>>> function and adding a new function TrimCLOG() that does everything
>>>> StartupCLOG() used to do.
>>>
>>> +1 ... I found that overly cute also.
>>
>> It would have been even easier to move StartupCLOG() later, but then
>> we'd need a big comment explaining why CLOG starts up at one point and
>> subtrans starts up at another point, since that is very confusing way
>> of doing things. I wrote it that way first and it definitely looks
>> strange.
>>
>> It's much easier to understand that StartupCLOG() is actually a no-op
>> and that we need to trim the clog at the end of recovery in all cases.
>
> If it's a no-op, why have it at all? I know we have a bunch of places
> in the code where we have empty stubs where there used to be
> initialization or cleanup code, but I've never found that particularly
> good style. If something no longer requires initialization in a
> certain place, I think we should nuke the whole function.
It is a no-op for exactly the same reason other similar functions are
no-ops - it used to do something but now does not.
Anyone seeing StartupSubtrans and StartupMultiXact but no StartupClog
will immediately ask "why?".
IMHO it's easier to have an obviously named function than a comment -
its less invasive for a backpatch as well.
I'm following current code style. If you wish to change that, feel
free to change this and all other locations that do this. Until then,
doing this makes most sense and follows current coding style.
If I had done it the way you suggest, I don't doubt someone would say
in about 6 months "Which idiot removed StartupClog()?".
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services