Re: bug? Vendor Invoice update

["Chris Travers" <..hidden..>]

Just to clarify this bug will only occur under two very specific conditions:

1)  A pre-upgrade (to lsmb 1.2.x) invoice is reposted once or
2)  A post-upgrade invoice is reposted twice.

On Fri, Oct 17, 2008 at 7:21 PM, david <..hidden..> wrote:

> comments, which I hope are constructive ;-)
>
> * "enforce transaction reversal" is set to "NO" by default. Surely "Yes" would
> be better? I didn't actually realise the implications until I fell foul of them.
> Luckily I had a backup to go to.

Actually, it is set to "Yes" be default provided that you arent
upgrading a system with it turned off.

SQL-Ledger sets it to "No" by default so we preserve settings generally.

However, I think we should go forward by turning it on during the 1.3
upgrade process.

>
> * having a function which causes a database failure is the real problem. If that
> button corrupts the system (which I can confirm that it does) then the user
> shouldn't be able to get past the warning.

I completely agree with you on this point.   I can also tell you that
we take this sort of problem seriously.

There are, however, two constraints that we face:

1)  We only apply bug fixes to stable branches.  All new features and
general behaviour changes (with *very* few exceptions, usually when an
advertised feature never works properly in the first place even under
ideally supported conditions) occur in the main development branch.
This makes fixing a bug like this very difficult.  However, I do have
an idea at present that may prevent this error going forward (will
test and apply for 1.2.18).  The fix in sql/fixes/ is not something we
can simply apply during an upgrade process however.

In fact, the only case I can think of where we had major behavior
change in 1.2.x was in fixing some of the pricematrix bugs.

2)  As of 1.2 we only  barely began the process of refactoring the old
code.  This bug occurs because of a combination of issues with the old
code and attempts to prevent bigger problems from the old code.

>
> * I thought I had read ALL the docs, but I missed the release notes. Perhaps all
> the docs including various readme's install notes release notes etc could be
> gathered into the one place, in one directory at one level. Right now they are
> scattered around a bit and confusion can result (I'm prone to confusion.. I
> understand these things!). Maybe even README.d
>
> * I've looked at transactions_pkey_repost.sql... unfortunately my knowledge of
> SQL is poor .... does that prevent future repostings from crashing or does it
> correct corruption caused by past reposting? (aside from the COGS problem etc)

Ok.  Let me explain the technical details of this bug and what the fix does:

One of the problems that could happen with SQL-Ledger databases is
that id's could be re-used.  Often this happens because of someone
either tampering with the sequence or with a bad backup from older
versions of PostgreSQL.  Then this would happen, joins would become
ambiguous and hence one would have to clean up the database by hand
(and often one would have to go back and manually re-enter any
affected invoices).

The basic problem that one has is that one gets an id in, say, ap with
the same value as one in ar or gl.  Thus the acc_trans records for
each join against both records, and the financial impacts of both
transactions double.  This usually cannot be fixed without going back
to the paper records.

So in 1.2, we added a table called "transactions" which prevents id
reuse.  Every insert to ar, ap, gl, etc. also causes its id to be
inserted into "transactions."  Since SQL-Ledger has a rather liberal
use of that sequence, we apply it to warehouses, projects, customers,
vendors, etc. in order to avoid join projection problems.
Unfortunately, the way that this worked had a bug in it (id's would
increment both places when the sequence is called to generate the
number), so if you insert an ar record which generates an id of 11, an
id of 12 will be inserted into transactions, and the next one into a
financial table would be 13.

So when we import records, they have the same id both places, but when
we generate them, a bug causes the transactions table to increment
each id again.

Ok, so suppose you repost a vendor invoice.  What happens is that
acc_trans, ap, and such get deleted from and then new inserts with the
same transaction id (ap.id, acc_trans.trans_id).  On this re-insert,
the id is re-used and so the same id is saved in transactions (not the
one higher) because the sequence is not hit.  A database commit occurs
after the deletes because SQL-Ledger lets you delete invoices
separately from this process.

Ok.  So what happens now is that you delete from all the tables except
for "transactions."  The database transaction is committed.  Then you
try to insert records with the same id.  Since the id is already in
"transactions," the insert fails and the database rolls back to the
last commit (after the delete).

This fix is a minimal fix to correct this problem.  What it does is
add a database macro which says "when you delete from these tables,
also go ahead and delete from transactions too where the id's match."
Thus  the id reuse error cannot occur in this context.

Long-term, I see two aspects to preventing any other issues which may
have similar causes:
1)  We need to get rid of the necessity to repost invoices.  We should
have a reverse-and-post-new function which allows for proper reversal
(voiding) of invoices and posting of new ones without any additional
work.  There is a little work to be done here in defining
requirements.  We also need to move the invoice notes to the new
framework which will allow notes to be added without reposting.

2)  We need to get all of the code moved over to the new framework.
This will allow much better control over database transaction
commitment and avoid what should be atomic transactions from being
broken up into multiple smaller transactions.
>
> * I have been reposting invoices with changes to the "notes" field.... I still
> get a warning not to repost but it doesn't crash when I proceed. Should I assume
> that changing non-financial fields such as notes and memos does not cause a
> problem with the database?

No.  You will get this error if you repost *new* invoices more than
once.  (This is why it did not get reported until quite a while after
1.2.0 was released.)  I.e. first repost on a post-migration invoice
succeeds, second one fails.

>
> * am I right to assume that the problems do not apply to "Post AS New"?

Correct.

Also, no apologies needed. :-).

Best Wishes,
Chris Travers