gabordemooij / redbean

ORM layer that creates models, config and database on the fly
https://www.redbeanphp.com
2.31k stars 279 forks source link

Transactions and R::begin with $frozen = false #706

Closed marios88 closed 5 years ago

marios88 commented 5 years ago

Currently running R::begin() on a fluid database will be silently ignored. I suggest it logging a php warning

plus the manual should read (https://www.redbeanphp.com/index.php?p=/database)

From

Note about auto-commits:
Many databases automatically commit after changing schemas, so make sure you test your transactions after R::freeze(true); ! 

To

In order to start a transaction the database *must* be frozen since many databases automatically commit after changing schemas.  
It is not possible to rollback transactions on fluid databases.
Lynesth commented 5 years ago

IIRC some database (Postgre ?) can rollback DDL so the original comment is not wrong.

Edit: I am not an a computer, does Redbean just exit the R::begin function if not frozen ?

marios88 commented 5 years ago

Line 1651 https://github.com/gabordemooij/redbean/blob/913c8072a13105d7d702ee88e762796773d212fe/RedBeanPHP/Facade.php#L1649-L1654

IIRC some database (Postgre ?) can rollback DDL so the original comment is not wrong.

It seems wrong in the context of the manual

Lynesth commented 5 years ago

According to this and multiple other website it should work. An excerpt:

Like several of its commercial competitors, one of the more advanced features of PostgreSQL is its ability to perform transactional DDL via its Write-Ahead Log design. This design supports backing out even large changes to DDL, such as table creation. You can't recover from an add/drop on a database or tablespace, but all other catalog operations are reversible.

I really don't have the ability to test that it works, so if your can, just remove those returning lines from R::begin and R::commit and try it out!

I guess we could talk about how it should be implemented in RedBean, since returning from the function if not frozen would not make it work. Maybe move that check in the adaptor and throw a NOTICE if needed ?

gabordemooij commented 5 years ago

I think I agree with this, the effect of starting a transaction should depend on the database since some DBs will support DDL transactions and others won't.

marios88 commented 5 years ago

Maybe move that check in the adaptor and throw a NOTICE if needed ?

I think I agree with this, the effect of starting a transaction should depend on the database since some DBs will support DDL transactions and others won't.

That sounds right, but i think it should throw a WARNING, since the developer tried to start a transaction on an unsupported database and no transaction will be executed

Lynesth commented 5 years ago

@marios88 @gabordemooij We should simply let the transaction happen wether the db is frozen or not:

I think, it should be allowed all the time without any PHP NOTICE or WARNING because it could be intended behavior and doesn't change anything (see points above and excerpt below).

From MariaDB under Transactions:

DDL statements (CREATE, ALTER, DROP) and administrative statements (FLUSH, RESET, OPTIMIZE, ANALYZE, CHECK, REPAIR, CACHE INDEX), and LOAD DATA INFILE, cause an implicit COMMIT and start a new transaction.

marios88 commented 5 years ago

@Lynesth valid points, indeed that seems like the best course of action

gabordemooij commented 5 years ago

@Lynesth @marios88 I added a function

R::setAllowFluidTransactions()

to make this possible. The default behavior will remain though, just to make sure transactions in fluid mode behave the same on every platform and because of backward compatibility.

Lynesth commented 5 years ago

@gabordemooij Sounds good to me. However, could you try to rollback in the tests instead of commit ? Because the current test should work without FluidTransactions while the rollback part makes it more specific.

gabordemooij commented 5 years ago

aec6692aaacd3d24d7c47cee1d35c0be0de6ee91

gabordemooij commented 5 years ago

Added an additional test with rollback.