Обсуждение: msdtc with 32-bit app fails to resolve in-doubt or not-notifed transactions

Hi folks

I've found an issue with psqlODBC's MSDTC support and pgxalib.dll, where
a 32-bit application on a 64-bit server will intermittently leave
transactions in the "only failed to notify" state in MSDTC.

This occurs when:

- The application exits normally after its final ITransaction::Commit
call returns but before MSDTC has invoked
ITransactionResourceAsync::CommitRequest on the psqlODBC-provided
IAsyncPG object; or

- When the application or server crash after MSDTC Phase I but before
Phase II.

In both these cases the resource manager is supposed to handle
transaction resolution. It uses pgxalib.dll for this as that's the
registered XA co-ordinator for the resource type.

I've been able to trace pgxalib.dll (which, btw, was painful, will
follow up on that) and found that XAConnection::xa_recover() is being
called on the transaction, as expected. It's calling into
XAConnection::ActivateConnection, where it fails to establish an ODBC
connection and bails out at the test at 142 after getting return code -1
from SQLDriverConnect(...).

http://msdn.microsoft.com/en-us/library/ms716219(v=vs.85).aspx

suggests that this is SQL_ERROR. pgxalib.dll doesn't call SQLGetDiagRec
or SQLGetDiagField to get any details and log them; I'll submit a
separate patch for that.

It took me a while to figure it out, but SQLDriverConnect is failing
because it's using the name of the 32-bit driver, since it got the DSN
from a 32-bit application. So there's no such driver as far as the
64-bit application is concerned.

(It didn't help that I couldn't enable system-wide ODBC tracing on the
system for unrelated and annoying as-yet-unresolved reasons with the
ODBC driver manager).

Anyway - it looks like it'll be necessary to figure out in pgxalib.dll
when this is happening and remap the driver name. That seems pretty
crude, though, so I'm looking for better ideas.

I'll follow up when it's not midnight with:

- a patch to add proper error diagnostics in pgxalib.dll on connection
failure;

- results of testing a hack that just mangles the dsn connection string
manually, as a proof of concept to show that this is really the issue; and

- If I can figure out how to do it the right way (as opposed to just
abusing a breakpoint to set the lvalue on return like I ended up doing),
some documentation on how to turn pgxalib tracing on.


As part of this I've been wondering whether it's possible to deal with
that exit race condition. I'm not sure how to tackle that - I don't
speak fluent COM or OLE. Do you think it'd be legal to delay in the
IAsyncPG dtor until either we confirm commit of an a tx we know is in
flight or we hit a (short) timeout?

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

(2014/06/21 1:03), Craig Ringer wrote:
> Hi folks
>
> I've found an issue with psqlODBC's MSDTC support and pgxalib.dll, where
> a 32-bit application on a 64-bit server will intermittently leave
> transactions in the "only failed to notify" state in MSDTC.
>
> This occurs when:
>
> - The application exits normally after its final ITransaction::Commit
> call returns but before MSDTC has invoked
> ITransactionResourceAsync::CommitRequest on the psqlODBC-provided
> IAsyncPG object; or
>
> - When the application or server crash after MSDTC Phase I but before
> Phase II.
>
> In both these cases the resource manager is supposed to handle
> transaction resolution. It uses pgxalib.dll for this as that's the
> registered XA co-ordinator for the resource type.
>
> I've been able to trace pgxalib.dll (which, btw, was painful, will
> follow up on that) and found that XAConnection::xa_recover() is being
> called on the transaction, as expected. It's calling into
> XAConnection::ActivateConnection, where it fails to establish an ODBC
> connection and bails out at the test at 142 after getting return code -1
> from SQLDriverConnect(...).
>
> http://msdn.microsoft.com/en-us/library/ms716219(v=vs.85).aspx
>
> suggests that this is SQL_ERROR. pgxalib.dll doesn't call SQLGetDiagRec
> or SQLGetDiagField to get any details and log them; I'll submit a
> separate patch for that.
>
> It took me a while to figure it out, but SQLDriverConnect is failing
> because it's using the name of the 32-bit driver,

Oops, it's my mistake. It may be better to rewrite the code completely
using libpq APIs instead of ODBC APIs.

> since it got the DSN
> from a 32-bit application. So there's no such driver as far as the
> 64-bit application is concerned.
>
> (It didn't help that I couldn't enable system-wide ODBC tracing on the
> system for unrelated and annoying as-yet-unresolved reasons with the
> ODBC driver manager).
>
> Anyway - it looks like it'll be necessary to figure out in pgxalib.dll
> when this is happening and remap the driver name. That seems pretty
> crude, though, so I'm looking for better ideas.
>
> I'll follow up when it's not midnight with:
>
> - a patch to add proper error diagnostics in pgxalib.dll on connection
> failure;
>
> - results of testing a hack that just mangles the dsn connection string
> manually, as a proof of concept to show that this is really the issue; and
>
> - If I can figure out how to do it the right way (as opposed to just
> abusing a breakpoint to set the lvalue on return like I ended up doing),
> some documentation on how to turn pgxalib tracing on.
>
>
> As part of this I've been wondering whether it's possible to deal with
> that exit race condition. I'm not sure how to tackle that - I don't
> speak fluent COM or OLE. Do you think it'd be legal to delay in the
> IAsyncPG dtor until either we confirm commit of an a tx we know is in
> flight or we hit a (short) timeout?
>

> Oops, it's my mistake. It may be better to rewrite the code completely
> using libpq APIs instead of ODBC APIs.

I don't know if that'll work.

Nothing requires the transaction co-ordinator to be local to the
database. The ODBC app using the local MSDTC might be talking to an
Amazon RDS instance for all we know or care. All the local transaction
co-ordinator needs is to be able to talk to the database.

That becomes a problem if the user is using a pre-defined System DSN,
User DSN or File DSN rather than a string DSN like in the test case I've
been working with. At least with a string DSN we can parse out the
relevant details and make a libpq connection. With indirect DSNs we
can't do that (AFAIK), we'd need a way to ask ODBC / psqlODBC what the
connection parameters were in a way we could pass to libpq.

OTOH, the same issue seems to rule out just rewriting the driver name in
the connection string. We can't do that if it's indirect via a file,
user or system DSN, AFAIK.

I'll need to read a bunch of documentation and do some more testing
before I'm confident of any of that, though.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 06/21/2014 12:52 PM, Craig Ringer wrote:
>
>> Oops, it's my mistake. It may be better to rewrite the code completely
>> using libpq APIs instead of ODBC APIs.
>
> I don't know if that'll work.


... and in fact there are more issues, too.

If the DSN is using SSPI, MSDTC.exe probably won't be able to
authenticate to the DB as them. Same with any other kind of connection
that depends on the properties of the current user account.

Meh.

Given the low uptake of MSDTC and XA with Pg I think a lot of this can
just be documented away - "don't do that". So I'm not going to go
charging around trying to find 100% perfect solutions at the expense of
"good enough for the real world".

Where possible I'd like to find ways to produce useful errors though,
preferably at transaction enlistment time rather than commit phase II.
Suggestions would be valued.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

(2014/06/21 17:02), Craig Ringer wrote:
> On 06/21/2014 12:52 PM, Craig Ringer wrote:
>>
>>> Oops, it's my mistake. It may be better to rewrite the code completely
>>> using libpq APIs instead of ODBC APIs.
>>
>> I don't know if that'll work.

The driver doesn't pass original connection strings to XARMCreate().
It seems possible to pass LIBPQ connection strings.

>
> ... and in fact there are more issues, too.
>
> If the DSN is using SSPI, MSDTC.exe probably won't be able to
> authenticate to the DB as them. Same with any other kind of connection
> that depends on the properties of the current user account.
>
> Meh.

Yes it's a severe problem.
I'll try to check the good way to do it.

> Given the low uptake of MSDTC and XA with Pg I think a lot of this can
> just be documented away - "don't do that". So I'm not going to go
> charging around trying to find 100% perfect solutions at the expense of
> "good enough for the real world".
>
> Where possible I'd like to find ways to produce useful errors though,
> preferably at transaction enlistment time rather than commit phase II.
> Suggestions would be valued.

It's possible to reject transaction enlistments at XARMCreate() though
I'm not sure if it's possible to tell the cause clearly to users.

regards,
Hiroshi Inoue



--
I am using the free version of SPAMfighter.
SPAMfighter has removed 11041 of my spam emails to date.
Get the free SPAMfighter here: http://www.spamfighter.com/len

Do you have a slow PC? Try a Free scan
http://www.spamfighter.com/SLOW-PCfighter?cid=sigen

On 06/23/2014 08:34 AM, Inoue, Hiroshi wrote:
> (2014/06/21 17:02), Craig Ringer wrote:
>> On 06/21/2014 12:52 PM, Craig Ringer wrote:
>>>
>>>> Oops, it's my mistake. It may be better to rewrite the code completely
>>>> using libpq APIs instead of ODBC APIs.
>>>
>>> I don't know if that'll work.
>
> The driver doesn't pass original connection strings to XARMCreate().
> It seems possible to pass LIBPQ connection strings.

Ah, I misunderstood that then.

Great, that makes life easier.

>> If the DSN is using SSPI, MSDTC.exe probably won't be able to
>> authenticate to the DB as them. Same with any other kind of connection
>> that depends on the properties of the current user account.
>
> Yes it's a severe problem.
> I'll try to check the good way to do it.

Thanks for thinking about it.

>> Where possible I'd like to find ways to produce useful errors though,
>> preferably at transaction enlistment time rather than commit phase II.
>> Suggestions would be valued.
>
> It's possible to reject transaction enlistments at XARMCreate() though
> I'm not sure if it's possible to tell the cause clearly to users.

That'll be handy. As for telling users, we can always emit a message to
the Windows Event Log. Competent admins should be looking there anyway.


However, AFAIK we have no way to find out in advance if a connstring or
DSN passed by the driver to msdtc for use in pgxalib will actually be
usable - in particular we have no way to query `pg_hba.conf` to see (a)
how our current connection was authenticated and (b) whether the same
connection settings would apply for the pgxalib connection. For example,
I don't think there's any way to tell if our current user/host/db combo
has a special cased 'trust' entry in pg_hba.conf that means other combos
will require a password.

So I don't think there'll be any robust solution to this that doesn't
involve just documenting it away to some degree.

I suspect we'll land up wanting a multi-part solution:

- Use libpq in pgxalib and pass libpq connstrings generated from the dsn
by the odbc driver during tx enlistment to bypass the driver name issue;

- Provide a configuration option (an odbcinst.ini registry key?) to
allow the operator to override the driver-generated dsn/connstr used by
pgxalib with one the user knows will allow superuser connections to the
DB. If it exists this is used instead of any driver-generated connstring
for 2PC.

- Provide an ODBC parameter to set an escaped connection string for use
in XA management (?). Alternative to / override for the above registry
setting. This probably isn't necessary, as apps already have to set a
few registry keys to make XA work with MSDTC at all.

- Document XA configuration, noting that if SSPI is in use a connstring
*must* be configured. A user should be created for the purpose of XA 2PC
management and given precedence above SSPI connections in pg_hba.conf;
otherwise if SSPI works at all for NETWORKSERVICE you'd be giving every
NETWORKSERVICE app potential access to the db(s), which would be
undesirable.


I'm happy to write the required documentation, not least because I need
to document how to configure pgxalib with msdtc anyway - adding the
missing registry key for older versions, enabling XA support in msdtc in
component services, where to find the logs, how to enable msdtc's
tracing, how to enable pgxalib's tracing (if I can figure that out), how
to monitor for incomplete 2PC transactions, etc.

How *does* one correctly enable pgxalib tracing? I tried creating a
registry key "PostgreSQL" under "odbcinst.ini" in
HKLM\SOFTWARE\ODBC\ODBCINST.INI and adding a String value "Msdtclog"
with content '1' (no quotes). The key didn't seem to get found by
SQLGetPrivateProfileString though. So I'm not sure what the right way to
turn it on is.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

(2014/06/23 10:23), Craig Ringer wrote:
> On 06/23/2014 08:34 AM, Inoue, Hiroshi wrote:
>> (2014/06/21 17:02), Craig Ringer wrote:
>>> On 06/21/2014 12:52 PM, Craig Ringer wrote:
>>>>
>>>>> Oops, it's my mistake. It may be better to rewrite the code completely
>>>>> using libpq APIs instead of ODBC APIs.
>>>>
>>>> I don't know if that'll work.
>>
>> The driver doesn't pass original connection strings to XARMCreate().
>> It seems possible to pass LIBPQ connection strings.
>
> Ah, I misunderstood that then.
>
> Great, that makes life easier.
>
>>> If the DSN is using SSPI, MSDTC.exe probably won't be able to
>>> authenticate to the DB as them. Same with any other kind of connection
>>> that depends on the properties of the current user account.
>>
>> Yes it's a severe problem.
>> I'll try to check the good way to do it.
>
> Thanks for thinking about it.
>
>>> Where possible I'd like to find ways to produce useful errors though,
>>> preferably at transaction enlistment time rather than commit phase II.
>>> Suggestions would be valued.
>>
>> It's possible to reject transaction enlistments at XARMCreate() though
>> I'm not sure if it's possible to tell the cause clearly to users.
>
> That'll be handy. As for telling users, we can always emit a message to
> the Windows Event Log. Competent admins should be looking there anyway.
>
> However, AFAIK we have no way to find out in advance if a connstring or
> DSN passed by the driver to msdtc for use in pgxalib will actually be
> usable

XARMCreate() triggers xa_open() ( and xa_close()) probably so as to
check that the XADLL works. Currently xa_open() doesn't connect to
the database and simply saves the connection information for subsequent
xa_commit(), xa_rollback() or xa_recover() call. When original process
finished COMMIT/ROLLBACK PREPARED properly, No database access occurs
in msdtc process. We can change xa_open() so that it connects to the
database immediately and causes an error to XARMCreate() when the
connection fails.

regards,
Hiroshi Inoue

> - in particular we have no way to query `pg_hba.conf` to see (a)
> how our current connection was authenticated and (b) whether the same
> connection settings would apply for the pgxalib connection. For example,
> I don't think there's any way to tell if our current user/host/db combo
> has a special cased 'trust' entry in pg_hba.conf that means other combos
> will require a password.

On 06/24/2014 05:23 AM, Inoue, Hiroshi wrote:
>
> XARMCreate() triggers xa_open() ( and xa_close()) probably so as to
> check that the XADLL works. Currently xa_open() doesn't connect to
> the database and simply saves the connection information for subsequent
> xa_commit(), xa_rollback() or xa_recover() call. When original process
> finished COMMIT/ROLLBACK PREPARED properly, No database access occurs
> in msdtc process. We can change xa_open() so that it connects to the
> database immediately and causes an error to XARMCreate() when the
> connection fails.

That's enlightening. Thankyou very much.

I'll happily implement that and send in a patch. It may take a week or
two as I have some other projects on the boil, but hopefully it won't
take super long.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 06/24/2014 11:07 AM, Craig Ringer wrote:
> On 06/24/2014 05:23 AM, Inoue, Hiroshi wrote:
>>
>> XARMCreate() triggers xa_open() ( and xa_close()) probably so as to
>> check that the XADLL works. Currently xa_open() doesn't connect to
>> the database and simply saves the connection information for subsequent
>> xa_commit(), xa_rollback() or xa_recover() call. When original process
>> finished COMMIT/ROLLBACK PREPARED properly, No database access occurs
>> in msdtc process. We can change xa_open() so that it connects to the
>> database immediately and causes an error to XARMCreate() when the
>> connection fails.
>
> That's enlightening. Thankyou very much.
>
> I'll happily implement that and send in a patch. It may take a week or
> two as I have some other projects on the boil, but hopefully it won't
> take super long.

... and done.

Please merge branch fix-syswow64-msdtc from my repo at
https://github.com/ringerc/psqlODBC.git .

see: https://github.com/ringerc/psqlODBC/pull/2

Patch attached if you prefer that. See patch header and in-code comments
for details.

It may well be better to just rewrite pgxalib.dll to use libpq, but I
wanted to stay non-intrusive and simple if possible. For now at least.

I'm not sure the unicode vs ansi dance is necessary in the patch is
necessary - it's probably ok to just always use the unicode driver.
Again, though, deviating from what the driver already does as little as
possible seemed reasonable.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 06/25/2014 05:26 PM, Craig Ringer wrote:
> Please merge branch fix-syswow64-msdtc from my repo at
> https://github.com/ringerc/psqlODBC.git .

Oh, and while you do that, please avoid creating a merge commit. I hate
those merge commits when I browse the git history, they're just useless
noise. Please use "git rebase" to clean up the history in your local
repository first. You can use "git push --dry-run" before the actual
push to see what will be done. It will print the commit IDs that it
would push, something like this:

a87a7dc..de42ed4  master     -> origin/master

You can then do "git log a87a7dc..de42ed4" to see those commits.

Also, it would be good to reset the author/committer information in the
commit before pushing. Looking at the git history the other day, I was
quite surprised to see commits from Craig, as I didn't know he's a
psqlodbc committer. Then I realized that you had merged those commits
from Craig's github repository :-). You can do "git commit --amend
--reset" to reset them before pushing, but once you do that, should add
a line in the commit message itself to give credit for the author of the
patch.

That's what we do with the PostgreSQL main repository, anyway. We could
also decide to use the git's Author field to track the original author,
but at least the Committer field should reflect the actual psqlodbc
committer. I'm not sure how to change that without changing the Author
field, though.

- Heikki

On 06/26/2014 03:49 PM, Heikki Linnakangas wrote:
> On 06/25/2014 05:26 PM, Craig Ringer wrote:
>> Please merge branch fix-syswow64-msdtc from my repo at
>> https://github.com/ringerc/psqlODBC.git .
>
> Oh, and while you do that, please avoid creating a merge commit. I hate
> those merge commits when I browse the git history, they're just useless
> noise. Please use "git rebase" to clean up the history in your local
> repository first. You can use "git push --dry-run" before the actual
> push to see what will be done. It will print the commit IDs that it
> would push, something like this:
>
> a87a7dc..de42ed4  master     -> origin/master
>
> You can then do "git log a87a7dc..de42ed4" to see those commits.

I usually do a rebase to current master before pushing a feature branch
and requesting a merge. I'm surprised a merge commit was created -
someone must've committed other things between when I pushed and when it
was merged. There was a lot happening around then.

The committer may just:

    git merge --ff-only

to reject commits that aren't simply fast-forwards.


Or, as rebase merges are usually trivial, they can:

git checkout theremote/thebranch
git checkout -b thebranch
git rebase master
git checkout master
git merge --ff-only -

which will just rebase on top and merge.

> Also, it would be good to reset the author/committer information in the
> commit before pushing. Looking at the git history the other day, I was
> quite surprised to see commits from Craig, as I didn't know he's a
> psqlodbc committer.

Heh. Soon I shall have commit to all the drivers, then my evil plans
will finally be put in motion!

> That's what we do with the PostgreSQL main repository, anyway. We could
> also decide to use the git's Author field to track the original author,
> but at least the Committer field should reflect the actual psqlodbc
> committer. I'm not sure how to change that without changing the Author
> field, though.

I had a quick look without seeing anything. The two fields must exist
separately for some reason, though.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 06/25/2014 10:26 PM, Craig Ringer wrote:
> On 06/24/2014 11:07 AM, Craig Ringer wrote:
>> On 06/24/2014 05:23 AM, Inoue, Hiroshi wrote:
>>>
>>> XARMCreate() triggers xa_open() ( and xa_close()) probably so as to
>>> check that the XADLL works. Currently xa_open() doesn't connect to
>>> the database and simply saves the connection information for subsequent
>>> xa_commit(), xa_rollback() or xa_recover() call. When original process
>>> finished COMMIT/ROLLBACK PREPARED properly, No database access occurs
>>> in msdtc process. We can change xa_open() so that it connects to the
>>> database immediately and causes an error to XARMCreate() when the
>>> connection fails.
>>
>> That's enlightening. Thankyou very much.
>>
>> I'll happily implement that and send in a patch. It may take a week or
>> two as I have some other projects on the boil, but hopefully it won't
>> take super long.
>
> ... and done.
>
> Please merge branch fix-syswow64-msdtc from my repo at
> https://github.com/ringerc/psqlODBC.git .
>
> see: https://github.com/ringerc/psqlODBC/pull/2
>
> Patch attached if you prefer that. See patch header and in-code comments
> for details.

Here's the revised patch. I've tested this against the customer's
original test case and it now runs properly. MSDTC correctly recovers
abandoned tx's in which Phase I commit succeeded then the app exited /
crashed before Phase II completed on one or both sessions.

Updated patch attached:

git am -s 0001-Fix-driver-name-mismatch-between-32-bit-ODBC-app-and.patch

or pull my branch, above.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 06/26/2014 03:49 PM, Heikki Linnakangas wrote:
> On 06/25/2014 05:26 PM, Craig Ringer wrote:
>> Please merge branch fix-syswow64-msdtc from my repo at
>> https://github.com/ringerc/psqlODBC.git .
>
> Oh, and while you do that, please avoid creating a merge commit. I hate
> those merge commits when I browse the git history, they're just useless
> noise. Please use "git rebase" to clean up the history in your local
> repository first. You can use "git push --dry-run" before the actual
> push to see what will be done. It will print the commit IDs that it
> would push, something like this:
>
> a87a7dc..de42ed4  master     -> origin/master
>
> You can then do "git log a87a7dc..de42ed4" to see those commits.
>
> Also, it would be good to reset the author/committer information in the
> commit before pushing. Looking at the git history the other day, I was
> quite surprised to see commits from Craig, as I didn't know he's a
> psqlodbc committer. Then I realized that you had merged those commits
> from Craig's github repository :-). You can do "git commit --amend
> --reset" to reset them before pushing, but once you do that, should add
> a line in the commit message itself to give credit for the author of the
> patch.
>
> That's what we do with the PostgreSQL main repository, anyway. We could
> also decide to use the git's Author field to track the original author,
> but at least the Committer field should reflect the actual psqlodbc
> committer. I'm not sure how to change that without changing the Author
> field, though.

I did a little more looking.

It seems the general idea is to retain Author (so what's Committer for?)
and use the Signed-off-by stuff:

author: "git commit -s" (push/pull workflow) or "git format-patch -s"
(email workflow)

committer: "git commit --amend -s" (push/pull workflow) or "git am -s"
(email workflow)

Per:

http://stackoverflow.com/questions/2348911


I guess really it depends on what the history is supposed to be. I'm not
sure core is a good guide here, as the project pretty much adopted a
"git as a better CVS" workflow.

It looks like if you amend / rebase a commit, the committer field is set
to your details, but the author remains the same. That seems appropriate.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

(2014/06/26 16:49), Heikki Linnakangas wrote:
> On 06/25/2014 05:26 PM, Craig Ringer wrote:
>> Please merge branch fix-syswow64-msdtc from my repo at
>> https://github.com/ringerc/psqlODBC.git .
>
> Oh, and while you do that, please avoid creating a merge commit. I hate
> those merge commits when I browse the git history, they're just useless
> noise. Please use "git rebase" to clean up the history in your local
> repository first.

Hmm I'm using rebase when pulling from origin/master and using merge
when pulling from other branches. Is there a better way?
Is using merge with --ff-only option a better way as Craig mentioned?

 > You can use "git push --dry-run" before the actual
> push to see what will be done. It will print the commit IDs that it
> would push, something like this:
>
> a87a7dc..de42ed4  master     -> origin/master
>
> You can then do "git log a87a7dc..de42ed4" to see those commits.

OK thanks.

> Also, it would be good to reset the author/committer information in the
> commit before pushing. Looking at the git history the other day, I was
> quite surprised to see commits from Craig, as I didn't know he's a
> psqlodbc committer. Then I realized that you had merged those commits
> from Craig's github repository :-). You can do "git commit --amend
> --reset" to reset them before pushing, but once you do that, should add
> a line in the commit message itself to give credit for the author of the
> patch.
>
> That's what we do with the PostgreSQL main repository, anyway. We could
> also decide to use the git's Author field to track the original author,
> but at least the Committer field should reflect the actual psqlodbc
> committer. I'm not sure how to change that without changing the Author
> field, though.

git commit --amend seems to change the Committer field as Craig
mentioned. Is it OK to push the change by Craig with the Author Craig
and the Committer by me?

regards,
Hiroshi Inoue



--
I am using the free version of SPAMfighter.
SPAMfighter has removed 11136 of my spam emails to date.
Get the free SPAMfighter here: http://www.spamfighter.com/len

Do you have a slow PC? Try a Free scan
http://www.spamfighter.com/SLOW-PCfighter?cid=sigen

On 06/27/2014 07:04 AM, Inoue, Hiroshi wrote:
> (2014/06/26 16:49), Heikki Linnakangas wrote:
>> On 06/25/2014 05:26 PM, Craig Ringer wrote:
>>> Please merge branch fix-syswow64-msdtc from my repo at
>>> https://github.com/ringerc/psqlODBC.git .
>>
>> Oh, and while you do that, please avoid creating a merge commit. I hate
>> those merge commits when I browse the git history, they're just useless
>> noise. Please use "git rebase" to clean up the history in your local
>> repository first.
>
> Hmm I'm using rebase when pulling from origin/master and using merge
> when pulling from other branches. Is there a better way?
> Is using merge with --ff-only option a better way as Craig mentioned?

Hmm. I think you can rebase over origin/master after merging Craig's
branch. That should remove the merge commit.

>> Also, it would be good to reset the author/committer information in the
>> commit before pushing. Looking at the git history the other day, I was
>> quite surprised to see commits from Craig, as I didn't know he's a
>> psqlodbc committer. Then I realized that you had merged those commits
>> from Craig's github repository :-). You can do "git commit --amend
>> --reset" to reset them before pushing, but once you do that, should add
>> a line in the commit message itself to give credit for the author of the
>> patch.
>>
>> That's what we do with the PostgreSQL main repository, anyway. We could
>> also decide to use the git's Author field to track the original author,
>> but at least the Committer field should reflect the actual psqlodbc
>> committer. I'm not sure how to change that without changing the Author
>> field, though.
>
> git commit --amend seems to change the Committer field as Craig
> mentioned. Is it OK to push the change by Craig with the Author Craig
> and the Committer by me?

Fine with me.

- Heikki