Various issues were identified during a code review of the GTM-8182 enhancement (in GT.M V6.3-003).
1) REPLINSTMISMTCH error gets issued incorrectly when updates are done to multiple instances
using extended references.
2) A fatal GTMASSERT2 error is issued incorrectly in some cases.
3) A fatal SIG-11 (aka KILLBYSIGSINFO1/SIGMAPERR) error is issued incorrectly in some cases.
In fact, this does not even require multiple instances. Just one replication instance is enough.
4) An error in a database file turns instance freeze ON in the wrong instance in some cases.
5) Repeatedly using PEEKBYNAME on the journal pool when the latter is not up causes a memory leak.
6) A process incorrectly attaches a database region to an instance file at the first access to the
region instead of at the first update to the region when gtm_custom_errors is not defined.
These issues are tested in the r124 test (subtests named ydb312_*). See those tests for a more
detailed description of the actual use case.
The following code cleanup was first done.
It was noticed that a pointer to the gd_addr structure (i.e. the gld) is newly passed to various
functions (gvcst_init(), gv_init_reg() etc.). This is considered error-prone since we already have
access to the gld from the region (which is already a parameter passed to these functions) through
the reg->owning_gd field. The 2nd parameter of gvcst_init() and gv_init_reg() are now removed.
It makes the code simpler, more correct and in turn easier to understand/maintain. This first step
enabled various other fixes to be more easily done.
jnlpool_init.c used "gd_header" as a default when the passed in gld is NULL. This occurrence is
removed as it is error-prone (can cause incorrect REPLINSTMISMTCH errors to be issued) to rely on
a global variable. Instead all callers of jnlpool_init() are reworked so they use gd_header instead
whenever they don't have a specific gld to work with. Similar changes were made to repl_inst_get_name()
where using "gd_header" as a default is removed and the last parameter passed in callers is the gld
(i.e. NULL references have been replaced with the more correct and consistent "gd_header" global).
Any place where gd_header was newly introduced, it was examined if a "gvinit" call happened a little
later and if so that call was moved to before the jnlpool_init()/repl_inst_get_name() call. This way
we have a non-NULL gd_header passed to jnlpool_init() that way the gld has a pointer to the jnlpool
after the jnlpool_init(). This is needed so later calls to jnlpool_init() automatically recognize the
gld to jnlpool mapping and return right away instead of double attaching to the same jnlpool shared
memory. Examples where gvinit() call was moved are gtmsource.c, updproc_init.c. This meant some
MUPIP REPLIC -SOURCE commands (e.g. MUPIP REPLIC -SOURCE -SHOWBACKLOG) will now require the
ydb_gbldir/gtmgbldir env vars to be properly setup whereas previously they were okay without the env
vars being set. But that is not considered a big issue since most of those commands (like
MUPIP REPLIC -SOURCE -START, MUPIP REPLIC -SOURCE -SHUTDOWN etc.) anyways required the env var set.
The gld structures were next reworked slightly so we have flexibility to edit runtime-only gld-related
structures without affecting the gld format. We previously had space allocated in the gd_addr structure
for an 8-byte pointer (thread_gdi). This is now renamed as "gd_runtime" which is a pointer to a new
"gd_runtime_t" typed structure that in turn contains a "thread_gdi" member. This lets us add to the
"gd_runtime_t" type without worrying about the gld format being affected since the "gd_addr" structure
remains unchanged.
A new field in the gld gd_ptr->gd_runtime->jnlpool now points to the journal pool (if attached)
corresponding to a given gld. This way, given a gld we directly have access to the corresponding
journal pool without having to go through repl_inst_name() calls like was being done in V6.3-003.
This means once a replicated database file in a gld has opened the journal pool, opening other
replicated database files in the same gld is a lot faster. This also avoids calling jnlpool_init()
multiple times for the same gld from gvcst_init() in case CUSTOM_ERRORS_AVAILABLE is TRUE. Previously,
inside jnlpool_init() we used to find out that we have already opened the journal pool of interest
but why waste all of those cycles comparing file ids etc.
gd_ptr->gd_runtime is allocated even in case of a dummy gbldir since common code (e.g. aio_shim.h etc.)
relies on gd_ptr->gd_runtime being non-NULL be it a dummy or non-dummy gld.
Now that all this cleanup is done, it was straightforward (though not trivial) to fix the 6 issues
described above. The core of the changes is done in the following modules
gdsfhead.h
gvcst_init.c
gvcst_init_sysops.c
jnlpool_init.c
A lot of the logic in the above modules has been reworked/simplified as a result. Additionally,
the reworked changes continue to ensure that if multiple gld files point to the same replication
instance file, each of those gld files continue to point to only one journal pool i.e. only one
shmat() call to journal pool is done.
Various issues were identified during a code review of the GTM-8182 enhancement (in GT.M V6.3-003).
1) REPLINSTMISMTCH error gets issued incorrectly when updates are done to multiple instances using extended references. 2) A fatal GTMASSERT2 error is issued incorrectly in some cases. 3) A fatal SIG-11 (aka KILLBYSIGSINFO1/SIGMAPERR) error is issued incorrectly in some cases. In fact, this does not even require multiple instances. Just one replication instance is enough. 4) An error in a database file turns instance freeze ON in the wrong instance in some cases. 5) Repeatedly using PEEKBYNAME on the journal pool when the latter is not up causes a memory leak. 6) A process incorrectly attaches a database region to an instance file at the first access to the region instead of at the first update to the region when gtm_custom_errors is not defined.
These issues are tested in the r124 test (subtests named ydb312_*). See those tests for a more detailed description of the actual use case.
The following code cleanup was first done.
It was noticed that a pointer to the gd_addr structure (i.e. the gld) is newly passed to various functions (gvcst_init(), gv_init_reg() etc.). This is considered error-prone since we already have access to the gld from the region (which is already a parameter passed to these functions) through the reg->owning_gd field. The 2nd parameter of gvcst_init() and gv_init_reg() are now removed. It makes the code simpler, more correct and in turn easier to understand/maintain. This first step enabled various other fixes to be more easily done.
jnlpool_init.c used "gd_header" as a default when the passed in gld is NULL. This occurrence is removed as it is error-prone (can cause incorrect REPLINSTMISMTCH errors to be issued) to rely on a global variable. Instead all callers of jnlpool_init() are reworked so they use gd_header instead whenever they don't have a specific gld to work with. Similar changes were made to repl_inst_get_name() where using "gd_header" as a default is removed and the last parameter passed in callers is the gld (i.e. NULL references have been replaced with the more correct and consistent "gd_header" global). Any place where gd_header was newly introduced, it was examined if a "gvinit" call happened a little later and if so that call was moved to before the jnlpool_init()/repl_inst_get_name() call. This way we have a non-NULL gd_header passed to jnlpool_init() that way the gld has a pointer to the jnlpool after the jnlpool_init(). This is needed so later calls to jnlpool_init() automatically recognize the gld to jnlpool mapping and return right away instead of double attaching to the same jnlpool shared memory. Examples where gvinit() call was moved are gtmsource.c, updproc_init.c. This meant some MUPIP REPLIC -SOURCE commands (e.g. MUPIP REPLIC -SOURCE -SHOWBACKLOG) will now require the ydb_gbldir/gtmgbldir env vars to be properly setup whereas previously they were okay without the env vars being set. But that is not considered a big issue since most of those commands (like MUPIP REPLIC -SOURCE -START, MUPIP REPLIC -SOURCE -SHUTDOWN etc.) anyways required the env var set.
The gld structures were next reworked slightly so we have flexibility to edit runtime-only gld-related structures without affecting the gld format. We previously had space allocated in the gd_addr structure for an 8-byte pointer (thread_gdi). This is now renamed as "gd_runtime" which is a pointer to a new "gd_runtime_t" typed structure that in turn contains a "thread_gdi" member. This lets us add to the "gd_runtime_t" type without worrying about the gld format being affected since the "gd_addr" structure remains unchanged.
A new field in the gld gd_ptr->gd_runtime->jnlpool now points to the journal pool (if attached) corresponding to a given gld. This way, given a gld we directly have access to the corresponding journal pool without having to go through repl_inst_name() calls like was being done in V6.3-003. This means once a replicated database file in a gld has opened the journal pool, opening other replicated database files in the same gld is a lot faster. This also avoids calling jnlpool_init() multiple times for the same gld from gvcst_init() in case CUSTOM_ERRORS_AVAILABLE is TRUE. Previously, inside jnlpool_init() we used to find out that we have already opened the journal pool of interest but why waste all of those cycles comparing file ids etc.
gd_ptr->gd_runtime is allocated even in case of a dummy gbldir since common code (e.g. aio_shim.h etc.) relies on gd_ptr->gd_runtime being non-NULL be it a dummy or non-dummy gld.
Now that all this cleanup is done, it was straightforward (though not trivial) to fix the 6 issues described above. The core of the changes is done in the following modules
A lot of the logic in the above modules has been reworked/simplified as a result. Additionally, the reworked changes continue to ensure that if multiple gld files point to the same replication instance file, each of those gld files continue to point to only one journal pool i.e. only one shmat() call to journal pool is done.