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

Jaimin Pan jaimin.pan at gmail.com
Thu Sep 1 22:41:58 PDT 2016


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.

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.

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.

4) benefit: consistency
Reference 3)

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)





2016-09-02 4:54 GMT+08:00 Tomas Vondra <tomas.vondra at 2ndquadrant.com>:

> Hi there,
>
> I've started working on XL code a few weeks ago (working for 2ndQuadrant
> with Pavan). Also, we might have met at one of the many PostgreSQL
> conferences.
>
> I do have a proposals regarding XL code, which I'd like to discuss on
> this mailing list with the rest of the community.
>
> Based on a lot of merging I've done on XL code in the past few weeks, I
> think we should follow through with removing the #ifdef, originally
> proposed about a year ago in this tread:
>
> https://sourceforge.net/p/postgres-xl/mailman/message/34422764/
>
> Note: The thread actually discusses the alternative to only remove the
> non-XL ifdef branches and unify the ifdefs. I don't see a point in doing
> that - I propose to get rid of the ifdefs entirely.
>
> Let me explain why I think we should do that, and also address the
> concerns raised in that thread.
>
>
> 1) benefit: code readability
>
> I claim that the ifdefs make code difficult to read, as the ifdefs
> entirely break natural flow of the code, and you have to constantly
> check whether the current line is part of #ifdef or #ifndef. Not to
> mention that you can find nested ifdefs or even constructions like
>
>     #ifdef PGXC
>     #ifndef PGXC
>     ...
>     #endif
>     #endif
>
> And various other strangely nested #ifdef blocks.
>
> It also breaks indentation rules, because on many places the code does
> something like this:
>
> #ifdef PGXC
>     if (additional condition)
>     {
> #endif
>     if (condition)
>     {
>         ...
>     }
> #ifdef PGXC
>     }
> #endif
>
> which does not exactly improve the situation.
>
>
> 2) benefit: awkward code flows
>
> On various places the ifdefs are used to keep both the XL and non-XL
> versions of the code, which however leads to strange code flows and
> blocks all refactoring attempts. Not only does this further harm
> readability of the code, but it also leads to subtle issues due to
> variables not being initialized in some cases, and so on.
>
>
> 3) benefit: code correctness
>
> I don't know whether you ever tried to compile the code without the
> constants. I tried, and it crashed and burned quite horribly. I don't
> think we should keep code that does not even compile and is likely broken.
>
> I know it was mentioned that the code provides some sort of context, but
> I don't think relying on code that is likely broken is a good practice -
> there are much better ways to achieve that (see one of the following
> points).
>
>
> 4) benefit: consistency
>
> If you check the current 'git diff' against upstream, you'll find out a
> large portion of new changes is actually not annotated by the ifdefs,
> particularly in the new code.
>
> I personally take this as a hint that the ifdefs are not really seen as
> worth the effort, otherwise we'd use them more consistently.
>
>
> 5) concern: it makes merges easier / reduces conflicts
>
> If I understand it correctly, the idea is that when merging changes from
> upstream, the merge will find the non-XL code block and merge the change
> into that. I claim that this is (a) unrealistic in most cases, and (b)
> undesirable in the remaining ones.
>
> Firstly, it's unrealistic because many of the code blocks are actually
> too small to make this possible. Prime examples of this are changes in
> src/backend/access/transam/xact.c or src/backend/tcop/utility.c, which
> contain plenty of blocks like this:
>
>   #ifdef PGXC
>      ... one line of code ...
>   #else
>      ... one line of code ...
>   #endif
>
> If anything, this increases the likelihood of merge conflicts, and also
> makes them much less comprehensible.
>
> Secondly, I think it's a mistake to see merge conflicts as inherently
> bad. It's a sign something changed in the original code, and that maybe
> the new code needs a similar tweak. Sadly, if the merge manages to match
> the non-XL block, we're not going to notice during the merge but
> probably much later in the form of strange issues.
>
> I also don't think it's a good practice to encourage "stuffing changes"
> into existing #ifdef blocks with the goal of minimizing conflicts. The
> primary goal should be code that is logical (which also makes it easier
> to maintain an resolve conflicts).
>
>
> 6) concern: ifdefs are a good way to provide context
>
> As mentioned above, I think it's very dubious to rely on code that may
> be broken in various ways, because no one actually compiled it for years
> (not even talking about testing it).
>
> I also believe that when developing a feature, you generally care about
> the XL context (i.e. the code that surrounds the line you're editing),
> and not how the surrounding code looked like in the upstream years ago.
>
> There are much better ways to show the upstream context when needed,
> particularly "git diff" and "git difftool" commands. I'm using difftool
> with "meld" [http://meldmerge.org/] and I highly recommend it (there are
> alternative tools for OSX, Windows, or vimdiff). It's also great for
> "git mergetool" BTW.
>
> The ifdef approach also has the disadvantage that if effectively locks
> you to a single context - the one in the non-XL ifdef branches (so most
> likely PostgreSQL upstream). What if you want to compare XL with XC, or
> some other fork? Well, the ifdefs are likely different in those forks,
> so the diff will be cluttered with garbage.
>
>
> Objections?
>
>
> kind regards
>
> --
> Tomas Vondra                  http://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
> ------------------------------------------------------------
> ------------------
> _______________________________________________
> Postgres-xl-developers mailing list
> Postgres-xl-developers at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/postgres-xl-developers
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.postgres-xl.org/private.cgi/postgres-xl-developers-postgres-xl.org/attachments/20160902/8b220660/attachment.htm>


More information about the Postgres-xl-developers mailing list