Skip to content

Commit c7ade3a

Browse files
andinuxandinux
authored
refactor: move all savepoint management from shared layer to platform wrappers (#17)
* refactor: move all savepoint management from shared layer to platform wrappers Move database_begin_savepoint, database_commit_savepoint, and database_rollback_savepoint calls out of cloudsync_begin_alter and cloudsync_commit_alter (shared CRDT logic) into the platform-specific wrappers in cloudsync_sqlite.c and cloudsync_postgresql.c. This fixes the PostgreSQL "subtransaction left non-empty SPI stack" warning by ensuring SPI_connect() is called before the savepoint boundary, and creates architectural symmetry where shared code is pure business logic and all transaction management lives in platform wrappers.
1 parent 2ae6f53 commit c7ade3a

File tree

4 files changed

+74
-39
lines changed

4 files changed

+74
-39
lines changed

‎src/cloudsync.c‎

Lines changed: 2 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2273,16 +2273,10 @@ int cloudsync_begin_alter (cloudsync_context *data, const char *table_name) {
22732273
return cloudsync_set_error(data, buffer, DBRES_MISUSE);
22742274
}
22752275

2276-
// create a savepoint to manage the alter operations as a transaction
2277-
int rc = database_begin_savepoint(data, "cloudsync_alter");
2278-
if (rc != DBRES_OK) {
2279-
return cloudsync_set_error(data, "Unable to create cloudsync_begin_alter savepoint", DBRES_MISUSE);
2280-
}
2281-
22822276
// retrieve primary key(s)
22832277
char **names = NULL;
22842278
int nrows = 0;
2285-
rc = database_pk_names(data, table_name, &names, &nrows);
2279+
int rc = database_pk_names(data, table_name, &names, &nrows);
22862280
if (rc != DBRES_OK) {
22872281
char buffer[1024];
22882282
snprintf(buffer, sizeof(buffer), "Unable to get primary keys for table %s", table_name);
@@ -2311,7 +2305,6 @@ int cloudsync_begin_alter (cloudsync_context *data, const char *table_name) {
23112305
return DBRES_OK;
23122306

23132307
rollback_begin_alter:
2314-
database_rollback_savepoint(data, "cloudsync_alter");
23152308
if (names) table_pknames_free(names, nrows);
23162309
return rc;
23172310
}
@@ -2430,18 +2423,9 @@ int cloudsync_commit_alter (cloudsync_context *data, const char *table_name) {
24302423
rc = cloudsync_init_table(data, table_name, cloudsync_algo_name(algo_current), true);
24312424
if (rc != DBRES_OK) goto rollback_finalize_alter;
24322425

2433-
// release savepoint
2434-
rc = database_commit_savepoint(data, "cloudsync_alter");
2435-
if (rc != DBRES_OK) {
2436-
cloudsync_set_dberror(data);
2437-
goto rollback_finalize_alter;
2438-
}
2439-
2440-
cloudsync_update_schema_hash(data);
24412426
return DBRES_OK;
2442-
2427+
24432428
rollback_finalize_alter:
2444-
database_rollback_savepoint(data, "cloudsync_alter");
24452429
if (table) table_set_pknames(table, NULL);
24462430
return rc;
24472431
}

‎src/cloudsync.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
extern "C" {
1919
#endif
2020

21-
#define CLOUDSYNC_VERSION "0.9.200"
21+
#define CLOUDSYNC_VERSION "0.9.201"
2222
#define CLOUDSYNC_MAX_TABLENAME_LEN 512
2323

2424
#define CLOUDSYNC_VALUE_NOTSET -1

‎src/postgresql/cloudsync_postgresql.c‎

Lines changed: 49 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -763,26 +763,27 @@ Datum pg_cloudsync_begin_alter (PG_FUNCTION_ARGS) {
763763
const char *table_name = text_to_cstring(PG_GETARG_TEXT_PP(0));
764764
cloudsync_context *data = get_cloudsync_context();
765765
int rc = DBRES_OK;
766-
bool spi_connected = false;
767766

768-
int spi_rc = SPI_connect();
769-
if (spi_rc != SPI_OK_CONNECT) {
770-
ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("SPI_connect failed: %d", spi_rc)));
767+
if (SPI_connect() != SPI_OK_CONNECT) {
768+
ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("SPI_connect failed")));
771769
}
772-
spi_connected = true;
773770

774771
PG_TRY();
775772
{
773+
database_begin_savepoint(data, "cloudsync_alter");
776774
rc = cloudsync_begin_alter(data, table_name);
775+
if (rc != DBRES_OK) {
776+
database_rollback_savepoint(data, "cloudsync_alter");
777+
}
777778
}
778779
PG_CATCH();
779780
{
780-
if (spi_connected) SPI_finish();
781+
SPI_finish();
781782
PG_RE_THROW();
782783
}
783784
PG_END_TRY();
784785

785-
if (spi_connected) SPI_finish();
786+
SPI_finish();
786787
if (rc != DBRES_OK) {
787788
ereport(ERROR,
788789
(errcode(ERRCODE_INTERNAL_ERROR),
@@ -792,6 +793,14 @@ Datum pg_cloudsync_begin_alter (PG_FUNCTION_ARGS) {
792793
}
793794

794795
// cloudsync_commit_alter - Commit schema alteration
796+
//
797+
// This wrapper manages SPI in two phases to avoid the PostgreSQL warning
798+
// "subtransaction left non-empty SPI stack". The subtransaction was opened
799+
// by a prior cloudsync_begin_alter call, so SPI_connect() here creates a
800+
// connection at the subtransaction level. We must disconnect SPI before
801+
// cloudsync_commit_alter releases that subtransaction, then reconnect
802+
// for post-commit work (cloudsync_update_schema_hash).
803+
// Prepared statements survive SPI_finish via SPI_keepplan/TopMemoryContext.
795804
PG_FUNCTION_INFO_V1(pg_cloudsync_commit_alter);
796805
Datum pg_cloudsync_commit_alter (PG_FUNCTION_ARGS) {
797806
if (PG_ARGISNULL(0)) {
@@ -801,29 +810,55 @@ Datum pg_cloudsync_commit_alter (PG_FUNCTION_ARGS) {
801810
const char *table_name = text_to_cstring(PG_GETARG_TEXT_PP(0));
802811
cloudsync_context *data = get_cloudsync_context();
803812
int rc = DBRES_OK;
804-
bool spi_connected = false;
805813

806-
int spi_rc = SPI_connect();
807-
if (spi_rc != SPI_OK_CONNECT) {
808-
ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("SPI_connect failed: %d", spi_rc)));
814+
// Phase 1: SPI work before savepoint release
815+
if (SPI_connect() != SPI_OK_CONNECT) {
816+
ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("SPI_connect failed")));
809817
}
810-
spi_connected = true;
811818

812819
PG_TRY();
813820
{
814821
rc = cloudsync_commit_alter(data, table_name);
815822
}
816823
PG_CATCH();
817824
{
818-
if (spi_connected) SPI_finish();
825+
SPI_finish();
819826
PG_RE_THROW();
820827
}
821828
PG_END_TRY();
822829

823-
if (spi_connected) SPI_finish();
830+
// Disconnect SPI before savepoint boundary
831+
SPI_finish();
832+
824833
if (rc != DBRES_OK) {
834+
// Rollback savepoint (SPI disconnected, no warning)
835+
database_rollback_savepoint(data, "cloudsync_alter");
825836
ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("%s", cloudsync_errmsg(data))));
826837
}
838+
839+
// Release savepoint (SPI disconnected, no warning)
840+
rc = database_commit_savepoint(data, "cloudsync_alter");
841+
if (rc != DBRES_OK) {
842+
ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("Unable to release cloudsync_alter savepoint: %s", database_errmsg(data))));
843+
}
844+
845+
// Phase 2: reconnect SPI for post-commit work
846+
if (SPI_connect() != SPI_OK_CONNECT) {
847+
ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("SPI_connect failed after savepoint release")));
848+
}
849+
850+
PG_TRY();
851+
{
852+
cloudsync_update_schema_hash(data);
853+
}
854+
PG_CATCH();
855+
{
856+
SPI_finish();
857+
PG_RE_THROW();
858+
}
859+
PG_END_TRY();
860+
861+
SPI_finish();
827862
PG_RETURN_BOOL(true);
828863
}
829864

‎src/sqlite/cloudsync_sqlite.c‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -919,15 +919,20 @@ void dbsync_init1 (sqlite3_context *context, int argc, sqlite3_value **argv) {
919919

920920
void dbsync_begin_alter (sqlite3_context *context, int argc, sqlite3_value **argv) {
921921
DEBUG_FUNCTION("dbsync_begin_alter");
922-
923-
//retrieve table argument
922+
924923
const char *table_name = (const char *)database_value_text(argv[0]);
925-
926-
// retrieve context
927924
cloudsync_context *data = (cloudsync_context *)sqlite3_user_data(context);
928-
929-
int rc = cloudsync_begin_alter(data, table_name);
925+
926+
int rc = database_begin_savepoint(data, "cloudsync_alter");
927+
if (rc != DBRES_OK) {
928+
sqlite3_result_error(context, "Unable to create cloudsync_alter savepoint", -1);
929+
sqlite3_result_error_code(context, rc);
930+
return;
931+
}
932+
933+
rc = cloudsync_begin_alter(data, table_name);
930934
if (rc != DBRES_OK) {
935+
database_rollback_savepoint(data, "cloudsync_alter");
931936
sqlite3_result_error(context, cloudsync_errmsg(data), -1);
932937
sqlite3_result_error_code(context, rc);
933938
}
@@ -944,9 +949,20 @@ void dbsync_commit_alter (sqlite3_context *context, int argc, sqlite3_value **ar
944949

945950
int rc = cloudsync_commit_alter(data, table_name);
946951
if (rc != DBRES_OK) {
952+
database_rollback_savepoint(data, "cloudsync_alter");
947953
sqlite3_result_error(context, cloudsync_errmsg(data), -1);
948954
sqlite3_result_error_code(context, rc);
955+
return;
956+
}
957+
958+
rc = database_commit_savepoint(data, "cloudsync_alter");
959+
if (rc != DBRES_OK) {
960+
sqlite3_result_error(context, database_errmsg(data), -1);
961+
sqlite3_result_error_code(context, rc);
962+
return;
949963
}
964+
965+
cloudsync_update_schema_hash(data);
950966
}
951967

952968
// MARK: - Payload -

0 commit comments

Comments
 (0)