[Postgres-xl-developers] removing PGXC/XC ifdefs from XL code

Tomas Vondra tomas.vondra at 2ndquadrant.com
Fri Sep 2 07:24:54 PDT 2016


On 09/02/2016 07:41 AM, Jaimin Pan wrote:
> Hi,
>
> I think it is not a good idea to remove #ifdef. Based on my
> experience,  It provides big help to merge code from PostgreSQL.
>
> 1) benefit: code readability
>  I think it's not a big deal. Code is readabliity with the #ifdef which
> provided the context.
>

IMHO it is a big deal. It forces you to constantly think not about the 
code, but whether you're in #ifdef, #ifndef or "#else branch, etc.

It fail to see which context the ifdefs are supposed to provide.

If there is a block with non-XL code (in the #else branch of the 
#ifdef), supposed to provide context "how the code looked in plain 
PostgreSQL", I claim that this actually obscures the context that 
matters, the surrounding code that gets compiled and executed.

If there isn't such block, the ifdefs simply demarcate the XL code, and 
there are much better ways to do that (like, git diff).

 >
> 2) benefit: awkward code flows
> I agree that it will block some refactoring attempts. But you will gain
> more when you want to upgrade to new PostgreSQL version.
>

I've actually discovered a few bugs due to this. It also leads to 
compiler warnings, because it forces us to keep as much code unchanged 
including uninitialized/unused variables, etc.

> 3) benefit: code correctness
> I also find it can't compile successfully without XL(also XC) three
> years ago. Because not all XL/XC code are inside the #ifdef context.
> However, I don't think there is any reason to compile XL code without
> #ifdef.
>

If the code does not work, how do you know you can rely on it as a 
context to base your decisions on?

 >
> 4) benefit: consistency
> Reference 3)
>

That has nothing to do with that. Even the code that is properly in 
#ifdef / #else branches does not compile nowadays.

 >
> 5) concern: it makes merges easier / reduces conflicts
> It is the point I really care about. It provides big help to merge code
> from new PostgreSQL. Sometimes the new code from PostgreSQL is just
> before/after the XL code. The auto-merge tools often makes the merge
> wrong in logical. The #ifdef could remind you there is the XL code.
>
> 6) concern: ifdefs are a good way to provide context
> Same as 5)
>

As I wrote in my response to Suzuki-san, this does not match my 
experience. But my main point is that there are much better ways to get 
context during merge - particularly "git mergetool" with meld or some 
other visual merge interface.

Have you tried using it?

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services




More information about the Postgres-xl-developers mailing list